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

mdx-js/runtime: Avoid unstable element type to improve render performance #1655

Closed
marcustyphoon opened this issue Sep 15, 2021 · 10 comments · Fixed by #1760
Closed

mdx-js/runtime: Avoid unstable element type to improve render performance #1655

marcustyphoon opened this issue Sep 15, 2021 · 10 comments · Fixed by #1760
Labels
📚 area/docs This affects documentation 🏁 area/perf This affects performance 💪 phase/solved Post is done 🦋 type/enhancement This is great to have

Comments

@marcustyphoon
Copy link

marcustyphoon commented Sep 15, 2021

Heya! We're using the runtime package to live-render a preview window in netlify-cms-app. Naturally, the performance of this use case is never going to be amazing, but I believe I found a way to improve it noticeably.

Subject of the feature

mdx-js/runtime currently creates and calls a function that defines and returns an <MDXContent> component containing the desired content. I believe that, because this is a function that is defined on every render, its type is different on every render, so React's reconciler will always tear down the DOM component tree rather than diffing the DOM elements, if mdx-js/runtime is used live.

const suffix = `return React.createElement(
MDXProvider,
{components},
React.createElement(MDXContent, props)
)`

To test this, I made this change, and it seemed in a quick test to drop our render times by 4x. (From 4000ms... don't ask :P)

- React.createElement(MDXContent, props)
+ React.createElement(React.Fragment, null, MDXContent(props))

I'm anything but a React DOM expert or familiar with this codebase, so I imagine there's a much, much better way to do this.

Expected behavior

The MDX runtime creates the same component type tree when given the same input, as far as the React reconciler is concerned.

Alternatives

I can't think of any, besides "the performance is not quite as good as it could be."

@wooorm
Copy link
Member

wooorm commented Sep 15, 2021

I don’t think MDXContent({components}) is valid though? Now it’s a) not given all props, b) no longer passed to React.createElement, which I think is required for the framework to work properly?

@marcustyphoon
Copy link
Author

It's valid in the sense that the code does "work" and render the MDX. MDXContent is a functional component that just returns the entire MDX body as props, so calling it as a function and using its output as the children prop of a fragment gives the valid component tree. Is it correct code? Quite sure it's not, but it was the first way I thought of to test if the performance gain was real.

I imagine the correct method is something like defining MDXContent as a wrapper component in index.js that just renders its children, then having the dynamically created function invoke it with the MDX as the children prop? (I would test this, but I couldn't actually figure out how to build the repository...)

@wooorm
Copy link
Member

wooorm commented Sep 15, 2021

This repo is currently rather messy.
I’m porting xdm back into it. That repo is workable: https://github.com/wooorm/xdm#evaluatefile-options (a lot of names and options have changed)

Note: xdm returns MDXContent instead of rendering it, so that’s what you’re going for with your suggestions, right?

@marcustyphoon
Copy link
Author

Interesting! I saw that repo, but saw the line "No runtime" in the about, and figured it wasn't relevant :D I'll check it out.

Based on the "actual output" example in https://github.com/wooorm/xdm#use, I would say no to your question: like MDX, XDM is outputting code in which a function is defined that gets used as a react component, so the function identity will be different every time XDM is called, and React will unmount the component tree if it is rendered more than once.

Basically, it would be helpful if MDX/XDM had an API to output a render prop rather than a component as the highest level export of the output. Then, the runtime could use this render prop in a stable component.

@wooorm
Copy link
Member

wooorm commented Sep 15, 2021

“runtime” is a vague word which has been used for 2 different things, hence for xdm I used evaluate, which more clearly signals that it, well, “eval”s code

For your first post: what xdm does, is give you an MDXContent back, a function, you can use that yourself as MDXContent({ components }) if you want to.

For your last post, I don’t quite understand what you mean. A render prop that returns rendered content, isn’t that just, well, a component?
Can you clarify what you mean?

Could you take the example in: https://github.com/wooorm/xdm#optionsoutputformat. The first output is used for whole files, the second for evaluating/“runtime”, which then runs by doing const mod = new Function(output)(things, in, scope). mod is then the return of the second output.
How would you do it differently?

@marcustyphoon
Copy link
Author

marcustyphoon commented Sep 15, 2021

Yeah, sorry, I didn't explain very well!

For your first post: what xdm does, is give you an MDXContent back, a function, you can use that yourself as MDXContent({ components }) if you want to.

I see, so what I did is essentially what you have to do. Here's my shot at a full explanation:

See https://reactjs.org/docs/reconciliation.html#the-diffing-algorithm, first of all.

So the problem is that React's reconciliation algorithm has this very specific behavior that two components of "different types" will never be diffed; they will always be unmounted. Because the definition of the function MDXContent is inside the code that evaluate evaluates (I like your terminology), if you run evaluate multiple times and render the output (as one does on every keystroke when using MDX/XDM as a live preview engine), each resulting component tree will be unmounted instead of reconciled with the previous one.

To avoid this, no function that is defined within the evaluated code can be used as a React component. Calling these functions in a stable component body or as/in a stable component's prop is fine.

As you said, we can use the current behavior of MDX/XDM, where compile defines and exports a function, but do not actually use it in the React tree. This is what I did in testing, placing a React.Fragment in the React tree and calling the MDXContent function in the child prop. This worked (4x speedup; DOM is not touched when you rerender the output).

MDX's runtime is a React component that parses its children as MDX. It could be changed to do this, but it currently does not.

XDM's evaluate, like you said, gives you the MDXContent function, and one can use it as a component or as a render prop function. If XDM were merged into MDX, and MDX's runtime component-that-parses-its-children-as-MDX continued to exist, this Github issue would continue to apply to it. I don't see a place where XDM actually renders the MDXContent component, so this issue does not apply to XDM (though it might deserve some documentation, so that developers using evaluate for rendering are aware of this.)

@marcustyphoon marcustyphoon changed the title mdx-js/runtime: Output a stable type to improve render performance mdx-js/runtime: Avoid unstable element type to improve render performance Sep 15, 2021
@marcustyphoon
Copy link
Author

const suffix = `return React.createElement(
  MDXProvider,
  {components},
- React.createElement(MDXContent, props)
+ MDXContent(props)
)`

In fact, forget the fragment: this (in https://github.com/mdx-js/mdx/blob/main/packages/runtime/src/index.js) passes npm test.

@yume-chan
Copy link

I think you should wrap all the parsing, transformation, and generating function in a useMemo to avoid re-run all processes when input is not changed.

@marcustyphoon
Copy link
Author

Memoizing the input doesn't have anything to do with this PR, though I have of course done that!

Right now, when the input is changed, the entire DOM is unmounted and remounted. With this proposed change, the DOM is not unmounted, and only the changes that are made are applied to the DOM (i.e. a few keystrokes). See the React documentation here: https://reactjs.org/docs/reconciliation.html#the-diffing-algorithm.

wooorm added a commit that referenced this issue Oct 20, 2021
wooorm added a commit that referenced this issue Oct 20, 2021
This adds a note on calling `MDXContent` yourself when repeat-evaluating some MDX.

Closes GH-1655.
@wooorm
Copy link
Member

wooorm commented Oct 20, 2021

evaluate is now used here too, in the RC for version 2. See: https://v2.mdxjs.com. I’ve added a note to the docs on evaluate so other folks repeat-evaluating stuff can use it too. Thanks or reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 🏁 area/perf This affects performance 💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

4 participants