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

Slow virtualization of thoughts #2399

Open
raineorshine opened this issue Sep 16, 2024 · 1 comment
Open

Slow virtualization of thoughts #2399

raineorshine opened this issue Sep 16, 2024 · 1 comment
Assignees
Labels
performance Improve performance

Comments

@raineorshine
Copy link
Contributor

raineorshine commented Sep 16, 2024

I found that at 1000 thoughts, the interactions start to feel sluggish. This happens with the last thoughts, and it doesn't happen with the first thoughts. It starts at around ~500. After testing and looking at the code I figured this is because of the virtualization. The virtualization algorithm cuts the last thoughts but not the thoughts at the start. This makes the app performant for the start of the list but slow for cases where there are many thoughts and the user has scrolled there.

I tested with a hacked version of window virtualization (with hardcoded values), and it solved the slow interactions. This could really be the main thing to make em super performant. At first glance, it seems such a change is significant but tractable. You know more than I do about the algorithm. Do you see any roadblocks to making this happen?

Good find! I think there are probably some good opportunities to optimize the virtualization algorithm.

In the VirtualThought component, shimHiddenThought will remove a thought from the DOM after it becomes hidden (and its fade out animation completes):

const shimHiddenThought = useDelayedAutofocus(autofocus, {
delay: 750,
selector: autofocusNew => autofocus === 'hide' && autofocusNew === 'hide' && !!height,
})

However, when I look at the DOM it does not seem to be consistently working. So part of what we may be dealing with is a bug in the current virtualization algorithm. It might be good to check if shimHiddenThought is working at all in your test case.

Working

Steps to Reproduce

- US
  - States
    - Alabama
    - Alaska
    - Arizona
    - Arkansas
    - California
    - Colorado
    - Connecticut
    - Delaware
    - Florida
    - Georgia
    - Hawaii
    - Idaho
    - Illinois
    - Indiana
    - Iowa
    - Kansas
    - Kentucky
    - Louisiana
    - Maine
    - Maryland
    - Massachusetts
    - Michigan
    - Minnesota
    - Mississippi
    - Missouri
    - Montana
    - Nebraska
    - Nevada
    - New Hampshire
    - New Jersey
    - New Mexico
    - New York
    - North Carolina
    - North Dakota
    - Ohio
    - Oklahoma
    - Oregon
    - Pennsylvania
    - Rhode Island
    - South Carolina
    - South Dakota
    - Tennessee
    - Texas
    - Utah
    - Vermont
    - Virginia
    - Washington
    - West Virginia
    - Wisconsin
    -  Wyoming
      - a
        - b
  1. Set the cursor on US → States → Wyoming → a → b
  2. Refresh the page

Current & Expected Behavior

The tree nodes above Wyoming in the DOM have been shimmed:

<div
  aria-label="tree-node"
  class=""
  style="
    position: absolute;
    left: 36px;
    top: 982.998px;
    transition: left var(--durations-layout-node-animation-duration) ease-out,
      top var(--durations-layout-node-animation-duration) ease-out;
    width: calc(100% + 1em - 26px);
  "
>
  <div style="height: 33.7012px"></div>
</div>

Not Working

Steps to Reproduce

Same as above, just set the cursor to null, refresh, and then navigate to b.

Current Behavior

The thoughts above Wyoming are not shimmed. The entire thought component hierarchy is rendered even though it is not visible.

Expected Behavior

Hidden thoughts should not be rendered.

@raineorshine raineorshine added the performance Improve performance label Sep 16, 2024
@raineorshine
Copy link
Contributor Author

Aside from the bug, there's also the question of whether the current virtualization algorithm is adequate. I'm open to your thoughts, though I understand we may need to fix the bug in order to re-measure and see if it's still a bottleneck.

There's another important point about the virtualization algorithm: The siblings of the first visible thought (i.e. the cursor's parent) are intentionally not virtualized. This is to ensure that there is a smooth fade-in animation when moving up a level. If there is a way to virtualize those thoughts without affecting the animation performance, that could be an opportunity for improvement.

Steps to Reproduce

  1. Same as above, but set the cursor on a and refresh.
  2. Move the cursor up to Wyoming.

Current Behavior

  1. Unlike when refreshing with the cursor on b, in this example all of the hidden siblings of Wyoming are rendered fully.
  2. The siblings fade in very smoothly since they are already in the DOM.

Expected Behavior

Maybe this is expected?
Or maybe there is a way to ensure a smooth animation without having to render all the hidden siblings of the parent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improve performance
Projects
None yet
Development

No branches or pull requests

2 participants