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

fix($core): render static html via stream pipe (fix #1819) #2454

Closed
wants to merge 4 commits into from

Conversation

sgtcoolguy
Copy link

@sgtcoolguy sgtcoolguy commented Jun 19, 2020

Summary
This PR attempts to handle larger doc sites to avoid blowing up the NodeJs heap as seen in #1819, as well as provide a better experience by not just silently chewing through thousands of files. The PR attempts to achieve this by:

  • Using p-limit to limit the number of static HTML pages it attempts to render simultaneously. I arbitrarily chose 50.
  • Using bundleRenderer.renderToStream() and then using fs.writeStream() and stream.pipe(). This avoids rendering to a string that we then write to file. It should pipe it to the file as it goes (which I assume should reduce RAM usage)
  • Providing a nice progress bar that looks/feels slightly similar to the Webpack client./server ones earlier in the process.

Screen Shot 2020-06-19 at 10 05 55 AM

I should note that for our site with 1618 pages, I still had to increase the Node heap or it would sometimes blow up during the Webpack client/server bundling.

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:

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

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

While this adds an explicit dependency on p-limit, I did find it in the node_modules tree of my site, and I believe it was already deeply nested in the dependency chain of VuePress: @vuepress/core > copy-webpack-plugin > p-limit I aligned versions so it used ^2.2.0.

@sgtcoolguy sgtcoolguy changed the title Stream render (fix #1819) fix($core): render static html via stream pipe (fix #1819) Jun 19, 2020
@sgtcoolguy
Copy link
Author

cc @f3ltron I think from the linked issue, you may be the most relevant reviewer for this PR?

@sgtcoolguy
Copy link
Author

I'm going to close this PR. After using it for some time it has a significant performance degradation. Also, we worked around our underlying issue in that we moved off using Travis to build (it was killing the build because of no output for 10+ minutes)

@sgtcoolguy sgtcoolguy closed this Jun 30, 2020
@8zf
Copy link

8zf commented Jul 27, 2020

@sgtcoolguy , hello, I've just read about this, and I'm experiencing this js oom issue recently. Since this pr is closed, is there any other wok around on this?

@sgtcoolguy
Copy link
Author

@8zf basically I just had to "throw RAM" at Node: NODE_OPTIONS="--max-old-space-size=6144" vuepress build docs
Tweak that value to give it more RAM to work with. I have older CI nodes with only 8Gb RAM, so I had to get it to 6Gb like this - but if you have newer machines with more RAM you could crank that up.

@8zf
Copy link

8zf commented Jul 27, 2020

I've tried this, but in my case, there are 700+ pages(auto generated), which costs all 8gb ram I allocated and still OOM....
On the server machine(we got a service to generate and build pages on cloud), the situation could be better, I can allocate more ram, but I think that's not a solution in the long term😨.
So I have to add extra code to automatically replace the build/index.js file to build pages one by one.

I think this PR is a good way to solve problem like this, maybe you can add some configuration to vuepress to trigger build page by sequence/batch, or when large number of pages are detected :)

@sgtcoolguy
Copy link
Author

I opened this PR to try and solve the issue - but:

  • I still saw occasional OOM with webpack if I didn't crank up the RAM
  • It was way slower
  • I'm not a VuePress dev and don't really plan on continuing with the patch since it didn't even solve my own need and no one on the dev team commented or picked up the patch

So hey - you can fork and try the patch as a starting point, go right ahead - but I think it ultimately didn't solve the problem and instead introduced a huge performance regression. A simpler patch may work better for you just using something like p-limit to throttle the number of simultaneous renders.

@8zf
Copy link

8zf commented Jul 28, 2020

Maybe the dev team is too busy to handle so many problems. 😅

Really appreciate your advice :), I'll have a try

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

2 participants