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

Async Tree: Create a fast path for children that resolve sync #143162

Merged
merged 1 commit into from Feb 17, 2022

Conversation

JacksonKearl
Copy link
Contributor

Ref #140883

In my testing a folder with 50k nested files (touch {1..10000}.{js,map,ts,d.ts,jsx}) would render in ~3s with nesting off and 15s with it on. Profiling showed the bulk of this overhead was in creating/cancelling a ton of different timeouts for "slow element" states, and general Promise handling logic.

In the nesting case the children are known synchronously so all the Promise/timeout overhead is not needed. 079a688 creates a fast path for this case in explorer land to get the render time down to 10s, this PR extends that fast path into AsyncTree land to bring render time down to 5s.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! 👏

5s is still quite a bit of time. How can we bring it further down?

@joaomoreno joaomoreno added the tree-widget Tree widget issues label Feb 17, 2022
@joaomoreno joaomoreno added this to the February 2022 milestone Feb 17, 2022
@JacksonKearl JacksonKearl merged commit 563785e into main Feb 17, 2022
@JacksonKearl JacksonKearl deleted the jackson/fast-sync-path-in-async-tree branch February 17, 2022 16:08
@JacksonKearl
Copy link
Contributor Author

Hm... so the breakdown is roughly:

  • 1s for the file service to stat the directory with resolveChildren enabled (so it stats all 50k children too),
  • 200ms for the explorer model to be created,
  • 400ms for nesting to be computed,
  • 2s in the asyncTree's doRefreshSubTree (the sync fast path here could be extended upwards to this call in theory)
  • 750ms in asyncTree's refreshAndRenderNode, largely in filtering and the URI/string manipulation needed to do so

Easiest win is probably extending this optimization up to doRefreshSubTree. Next is maybe toFileStat (cc @bpasero), but there's nothing obvious going on that would be slow there. ls ing the files takes half a second already.

In terms of big scale architectural changes I think streaming the directory contents would be the solution, but that's a whole can of worms.

@bpasero
Copy link
Member

bpasero commented Feb 17, 2022

If there is a costly operation in the file service that is under our control then I can look at it. Btw node.js recently added fs.opendir to stream the contents:

https://nodejs.org/docs/v14.16.0/api/fs.html#fs_fs_opendir_path_options_callback

But this is not really exposed through our file services...

@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tree-widget Tree widget issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants