Skip to content
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

feat($core): Improve VuePress build time #2163

Merged
merged 7 commits into from
Mar 10, 2020
Merged

Conversation

kefranabg
Copy link
Collaborator

@kefranabg kefranabg commented Feb 2, 2020

Summary

These optimisations make sense on projects that contains a huge amount of pages to render.

Related to #1560

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:
    Performance enhancement

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Steps:

Improvement #1 : Render static pages in parallel instead of rendering one page after the other.
Improvement #2 : Run extendPageData enhancers in parallel

@kefranabg kefranabg added the status: WIP Work in progress label Feb 2, 2020
@meteorlxy
Copy link
Member

Tip: You can use "Create draft PR" for WIP PRs

@kefranabg
Copy link
Collaborator Author

@meteorlxy Yes I forgot about this one 😅 👍

@TribeWeb
Copy link

TribeWeb commented Feb 3, 2020

Hi @kefranabg. I have a project where the build was gradually slowing down until it fails (blows the stack) and I just tried your new code. It now renders all the pages in less than a second!

@itsxallwater
Copy link

itsxallwater commented Feb 5, 2020

@TribeWeb I'd be curious to know how many docs in your project and if you ran this code on top of 1.2 or 1.3. I've got a project with 2k+ docs and dropping this code onto 1.3, I'm still seeing lengthy build times (24 minutes). I also was getting "JavaScript heap out of memory" errors mid-build until I changed the build script to set node's max_old_space_size to 8192.

Keen to hear a bit more as I was working on a separate approach to pull in node's worker_threads which I had working under 1.2 and had dropped my build time from 1 hour to 7.5 minutes, though the move to 1.3 has created some issues.

@TribeWeb
Copy link

TribeWeb commented Feb 5, 2020

I'd be curious to know how many docs in your project and if you ran this code on top of 1.2 or 1.3

Hi @itsxallwater. It’s a much smaller project about 100 pages. I’m using it with Vuetify and moving from v1.5 to 2.2. Build time was seconds previous but now hitting out of memory errors until I tried the fix above. VuePress is 1.3. I keep hitting this problem sometimes for weird reasons. I isolated the build time problem on the previous version of Vuetify to one button and fixed by wrapping it in ‘’.

@bencodezen
Copy link
Member

Great work on this @kefranabg!

@meteorlxy
Copy link
Member

meteorlxy commented Feb 29, 2020

About improvement 1, #315 (comment) discuessed long time ago (in v0.x 😅). Let's check if it would break the cli output in current version.

@meteorlxy
Copy link
Member

Oh, so you removed the cli output in the PR.

Have you made some benchmark for these improvements? @kefranabg

@kefranabg
Copy link
Collaborator Author

@meteorlxy I'll update the description later to add the benchmark 😄On small projects like the vuepress doc, these changes are not that effective. But on projects that have like 600pages to render, it has an interesting impact.

There is still some improvements to do to improve the build time, probably on webpack config.

@kefranabg
Copy link
Collaborator Author

@meteorlxy

With more than 600 pages to render

Without optimization:
build time = 360 seconds
With optimizations:
build time = 120 seconds

}

const pagePaths = await Promise.all(pagePathsPromises)
Copy link
Member

Choose a reason for hiding this comment

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

From 91 to 97:

Suggested change
const pagePaths = await Promise.all(pagePathsPromises)
const pagePaths = await Promise.all(
this.context.pages.map(page => this.renderPage(page))
)

@kefranabg kefranabg merged commit 76da780 into master Mar 10, 2020
@kefranabg kefranabg deleted the fix/improve-build-time branch March 10, 2020 14:58
@Mister-Hope
Copy link
Contributor

Have you checked #2221? I am afraid this change will make this issue even worse.@kefranabg

@itsxallwater
Copy link

FWIW these updates took our build time down to ~10 minutes, and we're talking ~1500 docs with these plugins:

image

@Mister-Hope might be noteworthy but we're overloading the build command in the package.json to run as node --max_old_space_size=8192 ./node_modules/vuepress/cli.js build allowing node to use more memory.

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Mar 20, 2020

FWIW these updates took our build time down to ~10 minutes, and we're talking ~1500 docs with these plugins:

image

@Mister-Hope might be noteworthy but we're overloading the build command in the package.json to run as node --max_old_space_size=8192 ./node_modules/vuepress/cli.js build allowing node to use more memory.

I knew node can use more memory. But I am talking about memory leak. Before changing, I don't think it's normal that the memory keep rising during build.
@itsxallwater

@itsxallwater
Copy link

I don't disagree, just wanted to add context from our build experience.

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Mar 20, 2020

I don't disagree, just wanted to add context from our build experience.

Let me explain my idea more carefully, I agree that the build process of pages should be parrell intead of await each other. But there must be some GC issues while building. Your FR is nice, since the offcial accepted it, I want to remind the offical that this may make the GC issue even worse.

At least I am using even more memory in 1.4.0. You can't just talk that it not an issue since node can take more memory in use.

I think we should optimize the memory usage while building.

@itsxallwater
Copy link

I'm not arguing :)

More context for the thread at large--we've moved our build to a GitHub action and it's reliably coming in at 20 minutes.

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.

6 participants