Skip to content

perf: render pages in parallel #1094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

sanjaiyan-dev
Copy link
Contributor

Improving the performance by running the independent async functions in parallel ✈️.

Ref-: vuejs/vitepress#1320

Sorry if I made any mistakes :(

Copy link
Member

@Mister-Hope Mister-Hope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@sanjaiyan-dev
Copy link
Contributor Author

This is pretty bad, as it may blow up machine memory. Image my blog have 800+ pages, with each process taking 40MB, I will use 32GB memory.

This may heavily do negative preformance effect due to Node.js "Old Space" behavior, as memory taken greater than old space will be swapped to hardrive.

You may have a test at my blog https://github.com/Mister-Hope/Mister-Hope.github.io, and I am pretty sure you will get the process killed while rendering, and a site with around 200 pages may get stuck for hours.

You should test this with real project to provide a brechmark with pr like this. The SSG process may actually faster if you make a "Promise queue" and limit it to 5-20 process at same time.

Welcome to try that and show brenchamrk.

Hi,
Actually I thought node js runs the process in 4 default threads according to doc and it is also used for I/O tasks. And node js runs tasks concurrently or parallel according to device's capabilities.So I thought it won't break or hang the CPU by overloading because it will run certain tasks concurrently and parallel.

Ref link (about Node js parallel and concurrency) -: https://bytearcher.com/articles/parallel-vs-concurrent

Just my thoughts might be completely wrong

@sanjaiyan-dev
Copy link
Contributor Author

This is pretty bad, as it may blow up machine memory. Image my blog have 800+ pages, with each process taking 40MB, I will use 32GB memory.

This may heavily do negative preformance effect due to Node.js "Old Space" behavior, as memory taken greater than old space will be swapped to hardrive.

You may have a test at my blog https://github.com/Mister-Hope/Mister-Hope.github.io, and I am pretty sure you will get the process killed while rendering, and a site with around 200 pages may get stuck for hours.

You should test this with real project to provide a brechmark with pr like this. The SSG process may actually faster if you make a "Promise queue" and limit it to 5-20 process at same time.

Welcome to try that and show brenchamrk.

Hi,
Actually I thought node js runs the process in 4 default threads according to doc and it is also used for I/O tasks. And node js runs tasks concurrently or parallel according to device's capabilities.So I thought it won't break or hang the CPU by overloading because it will run certain tasks concurrently and parallel.

Ref link (about Node js parallel and concurrency) -: https://bytearcher.com/articles/parallel-vs-concurrent

Just my thoughts might be completely wrong

So I thought this won't break the CPU since it only uses the allocated amount of cores. 🥺

@Mister-Hope

This comment was marked as outdated.

@sanjaiyan-dev
Copy link
Contributor Author

I don't think a promise get gc before promise.all is fired.

However, I will show you some comparisons with my blog with your changes later.

Hope it worked as expected :)

@Mister-Hope
Copy link
Member

I already approved this. Meteorlxy is in charge to merge this and publish new version.

@sanjaiyan-dev
Copy link
Contributor Author

I already approved this. Meteorlxy is in charge to merge this and publish new version.

Sorry, Tks 💪🤝

@meteorlxy
Copy link
Member

The CI failed

@sanjaiyan-dev
Copy link
Contributor Author

The CI failed

@meteorlxy Extremely sorry, I think it failed due to some lint issues.

I used the GitHub web based editor to edit the code can you please help me to solve the issue by running a command:) Extremely sorry for the inconvenience.

@@ -72,21 +72,20 @@ export const build = async (
const { app: vueApp, router: vueRouter } = await createVueApp()
const { renderToString } = await import('vue/server-renderer')

if (spinner) spinner.text = `Starting to render pages ${chalk.magenta(app.pages?.map(vuePressPage => vuePressPage.path))}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the idea here, I would choose to remove page path, for my theme docs around 800 pages, if would be annoying with tons of path throwing to logs without even a line break.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the idea here, I would choose to remove page path, for my theme docs around 800 pages, if would be annoying with tons of path throwing to logs without even a line break.

if (spinner) spinner.text = `Starting to render pages ${chalk.magenta(app.pages.map(vuePressPage => vuePressPage.path)) + '\n'}`

Like this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\n

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike the idea here, I would choose to remove page path, for my theme docs around 800 pages, if would be annoying with tons of path throwing to logs without even a line break.

@Mister-Hope Sorry, is current change is ok ?

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
@meteorlxy meteorlxy force-pushed the sanjaiyan-async-render branch from eca1040 to a1d77c8 Compare December 8, 2022 13:52
@meteorlxy
Copy link
Member

Let's just remove the spinner text updates for now.

@meteorlxy meteorlxy changed the title Render pages asynchronously ✈️ perf: render pages in parallel Dec 8, 2022
@sanjaiyan-dev
Copy link
Contributor Author

Let's just remove the spinner text updates for now.

🙌💪

@meteorlxy meteorlxy merged commit 78f737c into vuepress:main Dec 8, 2022
Mister-Hope added a commit that referenced this pull request Dec 9, 2022

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
This reverts commit 78f737c.
meteorlxy pushed a commit that referenced this pull request Dec 9, 2022

Verified

This commit was signed with the committer’s verified signature.
bagder Daniel Stenberg
This reverts commit 78f737c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants