-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
[markdown-preview] Optimize re-rendering of content in a preview pane… #984
[markdown-preview] Optimize re-rendering of content in a preview pane… #984
Conversation
If you do the stress test I described, you might also notice another quirk of the |
OK, this PR now wanders a bit outside of its scope in order to fix the HTML-encoding bug I mentioned above. I figured there was no point in putting it in a separate PR. |
(bump) |
…especially syntax highlighting. This change brings in `morphdom` to allow us to be more efficient about how we apply the changes needed when the Markdown source re-renders. Instead of replacing all the content with every single change, we apply only the selective edits needed to adapt our existing markup to the target markup. Once this process is done, we introduce one `TextEditor` instance for each `pre` tag in the markup, then persist those editor elements in the rendered output so that we don't have to spend so much time creating and destroying editors. This is a _huge_ performance win, especially in documents with lots of code blocks. The editor instances are cached using the `pre` elements as keys, which is now possible because the `pre` elements themselves are preserved across re-renders. Editors are created when new `pre` elements appear, and are destroyed when they are no longer needed, change their contents whenever the contents of the `pre` changes, and change language modes whenever the code fence language identifier changes. This approach is now used no matter which Markdown renderer is employed; we manage syntax highlighting ourselves in all cases rather than letting `atom.ui.markdown` do it.
…because we don't need it for our use case. Helps performance.
…and make the pane appear earlier on first open.
47054ea
to
74589fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, but seems good to go :)
Co-authored-by: Maurício Szabo <mauricioszabo@users.noreply.github.com>
…especially syntax highlighting.
The Problem
Suppose you’re editing a large Markdown document with lots of code blocks. Actually, don’t suppose — give it a try. Take one of my Tree-sitter blog posts and put it into a Pulsar editor in Markdown mode. Now copy everything but the front matter and paste it at the end to double its size; you should have about 50 code blocks at this point.
Now open a Markdown preview pane with Ctrl+Shift+M. Make any change at all in the buffer and notice how irritable you suddently feel:
This is a big enough annoyance to make me want to hide the preview.
The culprit here is the amount of time we spend doing things with editors. As you can imagine, it’s not exactly cheap to create a new
TextEditor
instance for each code block whenever we re-render a preview. But the real pain is in the inserting-into-the-DOM phase; we pay a large cost in style recalculation because a very complex element is newly present on the page. The editor component commits one of the textbook “sins” of style recalculation: when the custom element is first added to the page, it reads its ownoffsetWidth
andoffsetHeight
to determine whether it’s visible yet. That’s a “read” operation interleaved with a bunch of “write” operations — which the browser hates because it can no longer defer those writes until the most convenient time.Years ago, the Atom developers realized they could use lots of new CSS tricks in order to speed up rendering and minimize style recalculation. My instinct is that a lot of those tricks work fine when the editor exists in its “natural” environment (taking up the entire width and height of a workspace pane) but not at all when the editor is in an unusual environment (existing among other elements in a static layout). But that’s just a guess.
I don’t know of an easy way to fix the underlying issue with TextEditor, but I do know of a way that we can make it not matter.
As I mentioned earlier, the main problem with
markdown-preview
is that the entire contents of the preview pane are replaced whenever the content changes. You don’t notice it because most of the content is the same, but it’s still a lot of work being done for what are usually very small changes. And it means that if you have 50 code blocks on a page, each one of those code blocks (and theirTextEditor
s) is disposed of and recreated every time you type something — even something that’s outside of the code blocks altogether.Description of the Change
If Markdown were costlier to render, we’d have had to fix this ages ago. It isn’t, so this is a problem that’s only ever cropped up in documents with lots of editors for syntax highlighting. So the fix is this:
TextEditor
to represent its content instead of thepre
tag. But hide thepre
tag instead of removing it, because otherwise the diffing is a pain.TextEditor
instances and their customatom-text-editor
elements so that they don’t change during re-renders. The only cost we pay to create a new one is when a new code block appears on the page. And since thepre
element references themselves don’t change — because we’re diffing HTML, not replacing it — we can cache the editors using thepre
elements themselves as the keys.This is pretty straightforward. The strangest part of this is the fact that the
atom-text-editor
elements will be in the output, but we have to pretend they aren’t there during the diffing phase; if we didn’t, they’d get destroyed as part of diffing, since they’re not in the Markdown output that we’re trying to imitate. Luckily, morphdom — our diffing library — makes this pretty easy. (And it adds only 5K to our bundle size.)This strategy for minimizing the cost of syntax highlighting is useful enough that we’ll want to use it in both parser modes, which means that we’ll no longer be asking
atom.ui.markdown
to do our syntax highlighting for us. It’ll be worth it.Quantitative Performance Benefits
My test was to create the document I described above — one of my blog posts, but doubled. Then I showed a Markdown preview pane and made some changes to the source outside of a text block.
Here's a “before” snapshot from a profiler. A single character change resulted in 1.5 seconds of editor lockup:
On the other hand, the “after” snapshot captures me typing an entire sentence; each keystroke results in under 100ms of work. That's still a lot, but then this is a huge document, and the dropped frames are barely noticeable. The tooltip in this screenshot describes just one of these narrower bars on the timeline.
Possible Drawbacks
The upside of our previous strategy (emptying the element and starting over every time) is that it cleans the slate between renders. Now that we’re trying to persist stuff, new edge cases emerge related to rendering.
I definitely had to do some experiments to make sure that editors are being destroyed when they ought to be, are being recreated when they ought to be, have their contents updated when they ought to, and change from one language to another when appropriate. Luckily, the existing spec suite was thorough enough to tell me exactly where I’d been remiss, and all the tests now pass.
Verification Process
Mess around in that document I had you make at the top of this ticket. Stress-test it on both
master
and this branch. Do the things I mentioned in the previous section; switch ajs
code block to ats
code block, then switch it to atswtf
code block (a language string that maps to no grammar) just to make sure it turns back into an ordinary editor with no syntax highlighting.Applicable Issues
I pre-announced this PR in #893 in this comment that nobody really asked for.
Release Notes