perf(mdx-loader): cache mdx/remark compiler instances by phryneas · Pull Request #4997 · facebook/docusaurus

phryneas · 2021-06-17T22:06:21Z

Motivation

With this change, a new mdx compiler will be created once per webpack config instead of once per config AND file. This can have a massive impact as for example in the case of https://github.com/phryneas/remark-typescript-tools where every instantiation of the compiler also meant an instantiation of the remark plugin and hence a new instance of the TypeScript compiler.

A very similar change has been made over in the mdx-loader in https://github.com/mdx-js/mdx/pull/1468/files and you can compare the introduced changes with the original compile implementation over at https://github.com/mdx-js/mdx/blob/907bd26d83287180c7155c0fb65671b7cb55c98d/packages/mdx/index.js#L44-L47

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

This brings memory consumption of the redux-toolkit docs with my plugin (without workarounds) down from about 6GB to 2.8GB, while the plugin with workarounds comes down to 2.4GB (as that shares the compiler instance between the server and client config, but I erred on the more secure side with this PR and kept those separate).

facebook-github-bot · 2021-06-17T22:06:24Z

Hi @phryneas!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

phryneas · 2021-06-17T22:18:07Z

Hmm. Seems like this would need @mdx-js/mdx to be bounced to expose createCompiler. Since the docusaurus repo doesn't even install on my operating system, I won't be able to do that. Could you please take over for that part?

I have 1.6.22 running here and that works well, so no major version bump necessary.

facebook-github-bot · 2021-06-17T22:56:02Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Josh-Cena · 2021-06-18T03:56:08Z

I guess you should try fetching upstream first, since it seems your branch is quite old... In the lastest master I see @mdx-js/mdx already bumped to 1.6.21.

phryneas · 2021-06-18T06:25:28Z

Damn, I could have sworn I checked out the docusaurus repo for the first time in my life yesterday!
But I must already have had a stale copy lying around. I'll take a look later.

# Conflicts: # packages/docusaurus-mdx-loader/src/index.js

slorber

Updated the PR

That looks like a nice idea overall, but I'm not sure it would work in the current form, as each cached compiler will capture the first filepath being compiled.

Maybe there's a way for a remark plugin to access current file path being processed, without passing it through options?

We can see the problem in practice by building the Docusaurus site:

3:45:39 PM: Error: Image ../assets/docusaurus-asset-example-banner.png used in docs/presets.md not found.
3:45:39 PM: at async Promise.all (index 0)

This image is not used in docs/preset.md, but in docs/guides/markdown-features/markdown-features-assets.mdx

slorber · 2021-06-18T13:47:55Z

+        ...DEFAULT_OPTIONS.remarkPlugins,
+        [
+          transformImage,
+          {staticDir: reqOptions.staticDir, filePath: this.resourcePath},


Using this.resourcePath here makes me think this PR is not good enough.

Our remark plugins need to be able to access current file path to check if relative image/asset links are valid etc.

Apparently we may be able to access it thanks to the transform(node,vfile) and vfile.history[0]: https://github.com/vfile/vfile#vfilehistory

Will see how to refactor the PR with it

Yeah, I just took another look at https://github.com/mdx-js/mdx/pull/1468/files and they completely removed the filePath: this.resourcePath from the options object and only used it as parameter in the second argument of compiler.process - so it seems to be the accepted standard way to use that instead of accessing filePath from the options object.

Josh-Cena · 2021-12-18T10:19:41Z

@slorber Let's try to get this merged for the 2.0 release as well. I personally don't know much about Webpack / Remark file resolution and it seems you have a solution in mind?

Josh-Cena

Nice, vfile fixes it!

netlify · 2022-01-20T07:36:14Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`31ae5f1`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6245516d7e5055000966bf90
😎 Deploy Preview	https://deploy-preview-4997--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-01-20T08:38:41Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	62
🟢 Accessibility	100
🟢 Best practices	100
🟢 SEO	100
🟢 PWA	90

Lighthouse ran on https://deploy-preview-4997--docusaurus-2.netlify.app/

slorber

This PR looks fine but this leads to a ton of new warnings in our case:
https://app.netlify.com/sites/docusaurus-2/deploys/61e9239e50d8f50008ce297d

Those are not brand new warnings apparently, also in prod:

It just became more visible now since we create like 16 MDX compilers due to all of our plugin instances.

I think we should try to fix these warnings first before merging this PR.

It might be warnings that only us have, due to some docs 🤷‍♂️ but let's figure this out

Josh-Cena · 2022-01-20T14:42:40Z

Mmm, true. Last time I inspected I didn't find anything useful about where we were missing these imports. Probably one in the deployment doc.

I'm actually surprised that this creates more warnings than before. I guess it's because every call to createCompiler creates a loader while calls to mdx are cached?

Josh-Cena · 2022-01-26T08:01:45Z

Okay, this problem is once again caused by the MDX infrastructure not present during blog feed generation 😅 Should be fixed by #6454

slorber · 2022-01-26T17:19:58Z

Okay, this problem is once again caused by the MDX infrastructure not present during blog feed generation 😅 Should be fixed by #6454

Nop, there were some missing Tab/TabItem imports that I fixed

Also we have an issue with the npm2yarn plugin, not sure why we only see this now. The warning could as well be related to upgrading MDX and we didn't notice.

As it's very verbose I'd rather get this fixed before we merge the PR

Josh-Cena · 2022-01-29T02:38:48Z

Also we have an issue with the npm2yarn plugin, not sure why we only see this now. The warning could as well be related to upgrading MDX and we didn't notice.

I suppose you are talking about the npm2yarn plugin automatically importing Tabs if they are not present? That seems to be the case for both files you fixed?

slorber · 2022-02-02T12:07:54Z

I suppose you are talking about the npm2yarn plugin automatically importing Tabs if they are not present? That seems to be the case for both files you fixed?

I didn't have time to investigate much, I just know that removing the remark plugin removes the warnings

Josh-Cena · 2022-03-31T07:00:58Z

Took a look and found the issue. It's because the npm2yarn plugin wrongly put the state outside the transformer, so it would share the same state across different MDX files, and cause later files to not have Tabs automatically imported.

Josh-Cena · 2022-03-31T07:42:51Z

Going to merge now! The behavior seems good. I have made sure we don't have stateful logic outside the transformer anywhere else, so caching shouldn't affect most users.

slorber · 2022-04-06T10:27:59Z

Thanks 👍

* (mdx-loader) only create mdx compiler once per webpack config * type fixes * fix path * remove assertion * docs: add missing Tab/TabItem imports * fixup Co-authored-by: slorber <lorber.sebastien@gmail.com> Co-authored-by: Josh-Cena <sidachen2003@gmail.com>

(mdx-loader) only create mdx compiler once per webpack config

7e44805

phryneas requested review from lex111 and slorber as code owners June 17, 2021 22:06

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 17, 2021

Merge branch 'master' into fix-mdx

00c24f4

# Conflicts: # packages/docusaurus-mdx-loader/src/index.js

slorber requested changes Jun 18, 2021

View reviewed changes

slorber changed the title ~~(mdx-loader) only create mdx compiler once per webpack config~~ fix(v2): mdx-loader perf: cache mdx/remark compiler instances Jun 18, 2021

phryneas mentioned this pull request Jun 26, 2021

Use a weakmap for the highlighters shikijs/twoslash#43

Merged

This was referenced Jun 28, 2021

OOMs on the Jest docusaurus site shikijs/twoslash#35

Closed

Add twoslash to docs & migrate contents remotion-dev/remotion#481

Merged

Josh-Cena added the pr: performance This PR does not add a new behavior, but existing behaviors will be more memory- / time-efficient. label Oct 30, 2021

Merge branch 'main' into fix-mdx

237d6ed

Josh-Cena changed the title ~~fix(v2): mdx-loader perf: cache mdx/remark compiler instances~~ perf(mdx-loader): cache mdx/remark compiler instances Nov 7, 2021

type fixes

f6b95a7

Merge branch 'main' into fix-mdx

f42082a

phryneas requested a review from Josh-Cena as a code owner December 18, 2021 10:22

Josh-Cena added this to the 2.0.0 milestone Dec 18, 2021

Josh-Cena added 2 commits January 20, 2022 15:13

Merge branch 'main' into fix-mdx

1005f2f

fix path

d9d99fb

Josh-Cena approved these changes Jan 20, 2022

View reviewed changes

Josh-Cena requested a review from slorber January 20, 2022 07:31

Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Jan 20, 2022

Merge branch 'main' into fix-mdx

fca06d0

Merge branch 'main' into fix-mdx

95d0e6b

remove assertion

51d17c5

slorber reviewed Jan 20, 2022

View reviewed changes

Josh-Cena mentioned this pull request Jan 22, 2022

refactor(mdx-loader): use vfile.path to access Markdown file path #6443

Merged

Merge branch 'main' into fix-mdx

b38e2a9

slorber added 3 commits January 26, 2022 16:57

Merge branch 'main' into fix-mdx

4972951

Merge branch 'main' into fix-mdx

cc0f233

docs: add missing Tab/TabItem imports

94d70de

Josh-Cena added 2 commits March 31, 2022 14:54

Merge branch 'main' into fix-mdx

59cb140

fixup

31ae5f1

Josh-Cena merged commit 949a72e into facebook:main Mar 31, 2022

Josh-Cena removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Apr 1, 2022

Josh-Cena mentioned this pull request May 11, 2022

fix(mdx-loader): use React.Fragment as fragment factory #7392

Merged

3 tasks

slorber mentioned this pull request Sep 29, 2022

After many rebuilds, "FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory" #8142

Closed

7 tasks

Uh oh!

Uh oh!

Conversation

phryneas commented Jun 17, 2021 • edited by Josh-Cena Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Uh oh!

facebook-github-bot commented Jun 17, 2021

Action Required

Process

Uh oh!

phryneas commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jun 17, 2021

Uh oh!

Josh-Cena commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phryneas commented Jun 18, 2021

Uh oh!

slorber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slorber Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

slorber Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Josh-Cena left a comment

Choose a reason for hiding this comment

Uh oh!

netlify Bot commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ [V2]

Uh oh!

github-actions Bot commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slorber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josh-Cena commented Jan 20, 2022

Uh oh!

Josh-Cena commented Jan 26, 2022

Uh oh!

slorber commented Jan 26, 2022

Uh oh!

Josh-Cena commented Jan 29, 2022

Uh oh!

slorber commented Feb 2, 2022

Uh oh!

Josh-Cena commented Mar 31, 2022

Uh oh!

Josh-Cena commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slorber commented Apr 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phryneas commented Jun 17, 2021 •

edited by Josh-Cena

Loading

phryneas commented Jun 17, 2021 •

edited

Loading

Josh-Cena commented Jun 18, 2021 •

edited

Loading

slorber left a comment •

edited

Loading

slorber Jun 18, 2021 •

edited

Loading

Josh-Cena commented Dec 18, 2021 •

edited

Loading

netlify Bot commented Jan 20, 2022 •

edited

Loading

github-actions Bot commented Jan 20, 2022 •

edited

Loading

slorber left a comment •

edited

Loading

Josh-Cena commented Mar 31, 2022 •

edited

Loading