-
-
Notifications
You must be signed in to change notification settings - Fork 622
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
refactor(core): Use iterative approach in recompute dependents #2821
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Preview in LiveCodesLatest commit: d570004
See documentations for usage instructions. |
7ec5ec5
to
b175b59
Compare
b175b59
to
6f60179
Compare
6f60179
to
baa1bc5
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.
This looks like a great improvement.
baa1bc5
to
349c1fc
Compare
349c1fc
to
4878f64
Compare
4878f64
to
1a899fe
Compare
1a899fe
to
862d7cc
Compare
862d7cc
to
423e5b7
Compare
423e5b7
to
409949a
Compare
409949a
to
d71f64d
Compare
d71f64d
to
ca597ed
Compare
ca597ed
to
e2a7395
Compare
e2a7395
to
be1e735
Compare
be1e735
to
70b0c1d
Compare
70b0c1d
to
d570004
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.
LGTM
Can you share the benchmark that you used to get the times in the PR description? This slowdown isn’t going to work for us, but I think I see how to optimize the iterative case, I want to prove it with the same benchmark though. |
Oh, would you be able to provide these benchmarks? I only used vitest test duration using stack limit size nodes (~12k) in a chained dependency configuration. I'd love to hear your ideas on how to make this faster. The original recursive solution suffers from stack overflow issues, so I'm keen on keeping an iterative approach. I tried both DFS and BFS and found they both have the same number of iterations but the DFS was more memory efficient. |
Oh, I found a typo in the PR description. The iterative DFS solution in around 250 ms in my testing, not 350 ms. |
We should be able to shuffle some things around to remove one of the visiting or visited sets. |
I'm not sure how to improve on the post-order DFS topological sort solution we have currently. I tried implementing Kahn's, but there's too much administrative work required to beat the current solution. |
Summary
Replaces the recursive topo search in recomputeDependents with an iterative one. An iterative approach theoretically is faster and does not suffer from stack overflow issues. This makes it possible to process very large atom graphs.
Benchmark
TODO: Currently the new iterative approach is slower than the recursive one.
Check List
pnpm run prettier
for formatting code and docs