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

perf(ui): workflow editor misc #7645

Merged
merged 25 commits into from
Feb 16, 2025
Merged

Conversation

psychedelicious
Copy link
Collaborator

@psychedelicious psychedelicious commented Feb 14, 2025

Summary

  • Optimize component and hook structure to reduce rerenders
  • Remove memoization on some selectors where it serves no purpose (bc the object will have a stable identity until it changes, at which point we need to re-render anyways)
  • Shift the connection error selector logic around to rely more on the stable identity of pending connection objects
  • Upgrade @reduxjs/toolkit to latest to take advantage of more efficient bulk query cache updates
  • Disable certain problematic animations (does not resolve pre-existing animated edge issue on Chrome)
  • Fix bad perf on the add node popover.

Related Issues / Discussions

Some discussion here: https://discord.com/channels/1020123559063990373/1149506274971631688/1339814928390950965

QA Instructions

Should be faster and not more broken. Check node connection logic.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions bot added the frontend PRs that change frontend files label Feb 14, 2025
@psychedelicious
Copy link
Collaborator Author

Goes great on FF, pear-shaped on Chrome. Unsure if the changes here are the cause or if the issue was there previously also. Will need to investigate before merging.

@psychedelicious psychedelicious marked this pull request as draft February 14, 2025 05:47
@github-actions github-actions bot added the frontend-deps PRs that change frontend dependencies label Feb 15, 2025
@psychedelicious
Copy link
Collaborator Author

Ok so chrome issue isn't new. Appears to be related to animation of the stroke-dashoffset CSS property. reactflow uses this to animate edges w/ a marching ant effect. When there are many many nodes visible at once, attempting to make a connection causes massive slowdown as Chrome flails about trying to animate the edge.

There's an ancient (over 10 years old) Chrome bug report for this: https://issues.chromium.org/issues/40958492

Dunno if there's a way to fix this, but at least we know it is not related to the reactflow upgrade or any recent changes.

@psychedelicious psychedelicious marked this pull request as ready for review February 15, 2025 13:57
@psychedelicious
Copy link
Collaborator Author

psychedelicious commented Feb 16, 2025

Some comparisons for the changes in this PR. Should all be non-functional, just being less wasteful with recomputation and improving component structure to reduce cascading rerenders.

Workflows testing was done in a heavy workflow (dozens of nodes and edges, all stacked up in the z-axis) unless otherwise noted. The comparisons are relative only - the actual timing numbers are much much better in a prod build, and without the heavy perf sampling. I did several comparisons, but the results here aren't cherry-picked. Just took the last one of each.

Loading gallery page

Note the series of light teal operations. Previously, RTKQ required individual dispatches for each query cache change. In the new version, you can batch these into a single dispatch. Saves about 1 frame @ 60fps - not crazy but still nice.

Before

before - loading gallery page

After

after - loading gallery page

Searching for nodes in the add node popover

This component had flawed debounce logic resulting in nasty stutters. Properly debounced now, zero stutters.

Before

before - typing in add node popover

After

after - typing in add node popover

Moving a node

A series of misc rendering and selector optimizations more than doubled the it/s for moving nodes.

Before

before - moving node

After

after - moving node

Typing in a node string field

More misc optimizations resulted in ~half the memory usage and ~doubled it/s.

Before

image

After

image

Copying a simple graph

Misc optimizations resulted in ~30% reducing in memory usage and time taken.

Before

image

After

image

Pasting a simple graph

Smaller perf improvement for pasting. But, memory characteristics are improved - note the lack of GC in the after image (the orange horizontal bars).

Before

image

After

image

@psychedelicious
Copy link
Collaborator Author

I dove a bit deeper into this chrome issue. Can't figure out how to work around it - looks like Chrome has some SVG rendering issues generally, not just stroke-dashoffset. I think this scenario is just something I never tested.

If workflows perf on Chrome becomes a pain point, we can reduce or remove these heavy CSS/style features:

  • Animations
  • Partial opacity
  • Shadows

Unfortunately the changes in this PR don't seem to really fix the issue. I thought I had fixed it - but then the issue returned without me making any other changes! I reverted the non-fix.

@psychedelicious psychedelicious force-pushed the psyche/perf/ui/workflow-editor branch from afac770 to 169cf99 Compare February 16, 2025 22:26
@psychedelicious psychedelicious merged commit 62e5b9d into main Feb 16, 2025
15 checks passed
@psychedelicious psychedelicious deleted the psyche/perf/ui/workflow-editor branch February 16, 2025 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files frontend-deps PRs that change frontend dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants