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

Improve performance for command and selection changes #443

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jonathonherbert
Copy link
Contributor

@jonathonherbert jonathonherbert commented Jan 9, 2025

What does this change?

We recently had a large article — 7000+ words and 242 elements, id 673f39b58f083ae557dd22af — which was slow to load and edit in production.

Using that article as a test case, this PR improves the performance of the elements plugin in two ways:

  • f62abf6: only calculate whether commands are valid once per transaction, storing the result in a plugin and using that to determine whether consumers need to rerender. Thanks @KaliedaRik for suggesting this approach as we paired to figure out what was slow!
  • 7076cba: only update elements on selection change if the selection concerns them (i.e. the selection moves within the node, or moves in or out of the node)

To help measure performance when profiling these changes, e857a1a adds a flag to disable performance-heavy features, which is useful for removing noise from serialisation, collab and dev tools code when profiling.

The table below gives the profiler script time for 30 keystrokes with the article above loaded in the demo page, with the above flag disabled. Tested on Chrome 131.0.6778.205 a 14-inch Macbook Pro, M1 Max, 2021:

Iteration 40dded8 f62abf6 7076cba
1 2224 1353 555
2 2230 1403 554
3 2381 1378 569
Average 2278.333333 1378 559.3333333
ms per keystroke 75.94444444 45.93333333 18.64444444

About 4x faster.

How to test

  • Reviewing commit-by-commit should make the changes as clear as possible. I'm really happy to pair on reviewing anything that isn't clear.
  • This PR adds unit tests for the selection logic, and updates our rerender tests in https://github.com/guardian/prosemirror-elements/blob/main/src/plugin/__tests__/plugin.spec.tsx to account for the new behaviour. They should pass. Existing integration tests test element wrapper movement.
  • Perhaps have a play with that large document on main and this branch, by copying and pasting the content from our CMS to the editor playground (yarn start.) Can you feel the difference? You can also test in Composer following the instructions in the readme — I've included examples of both in GIFs here:
Location Before After
Sandbox pme-slower pme-faster
Composer composer-slower composer-faster

How can we measure success?

Less pain editing large articles.

Is there anything more we can do?

On modern hardward, large articles with large numbers of elements like the above are still slow to load (something this PR does not attempt to address), and still have appreciable jank.

It looks like iterating over decorations, which we use to ensure nodeViews always update, takes up a fair few cycles, too. That might be another good place to start looking for performance improvements.

Screenshot 2025-01-09 at 14 12 40

Loading improvements look like they're connecting to rendering. A few strategies we could try:

  • Optimise the rendering of individual components.
  • Optimise the rendering of components generally, perhaps by avoiding a React root per-element and mounting elements in portals instead.
  • Implement a strategy for virtualising element rendering, so offscreen elements are not rendered until they're needed.

@jonathonherbert jonathonherbert requested a review from a team as a code owner January 9, 2025 14:14
@jonathonherbert jonathonherbert force-pushed the jsh/perf-for-node-predicates branch 2 times, most recently from cb07386 to d48f294 Compare January 9, 2025 14:53
@jonathonherbert jonathonherbert force-pushed the jsh/perf-for-node-predicates branch from 00bf585 to 4d870bc Compare January 9, 2025 15:01
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.

1 participant