-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
propagateContextChange visits all child fibers #20661
Comments
It visits all fibers because that's its purpose — to find the ones that depend on context. There are some ways we're planning to optimize the algorithm but they'll need significant refactors so don't count on them happening soon. For now, it's essentially bruteforce by design. However, it is possible that there might be some ways to optimize the function itself. It could very well be possible that it has become slower since the initial implementation due to some accidental deopt or compiler behaivor. So if you want to make a synthetic benchmark to stress-test it, and look for optimization opportunities, that would be appreciated. |
@gaearon I can have a try. There are two versions of files in react-resonciler(old.js and new.js). Which should I modify? |
Not sure which ones you'll get on build.. Try changing both and see which gets used? |
I opened a draft pull request #20786
beforeafter |
I noticed that the |
Thanks for the PR! I don't think this approach would work for us because then you have a performance cliff as soon as you have more than 32 contexts. We generally prefer predictable performance over "fast in some cases", because otherwise seemingly small changes can have disproportionate impact on the whole application, and it becomes very difficult to debug perf problems. |
After some rethinking, it seems a number array is good enough, since there won't be large number of contexts. I will have a try later |
This is not correct; we should expect that a large app (which is where this optimization is most needed) will have a large number of contexts. |
By the way, originally I referred to optimizing in terms of optimizing the brute force traversal itself. It's quite likely that there might be some engine deopts there, e.g. due to how GCC compiles code. So it would be good to look into the compile output and if there are ways to optimize it. |
I mean thousands of contexts. One number supports 32 contexts and a number array with length 32 supports 32 * 32 contexts. That's 1024 contexts and 32 is only one tree node size of a hash tree. Thus I believe this is quite enough to optimize the scenario and a hash tree is not necessary. |
I checked the output of production build and closure compiler did a great job. I tried the array method and it works fine although the implementation is not correct. Will this get merged ? I will finish it if it would get merged |
@intellild my guess is probably not, given that Andrew is already working on #20646 and #20890 . |
@markerikson Thanks. I read the PR and it seems they are not conflicting |
Hmm. Well, if it doesn't conflict there might be a chance, but that'd really be an @acdlite kind of question. |
Just out of curiosity, i wonder how this finding child fiber can be optimized. A fiber can be very deep deep down the branch, the official version traces from the origin of the context to each child, because each fiber has a If we go the opposite way, suppose we store all fibers referencing the context in the context fiber's
Then maybe this will save work in Questions: just a note here, seems |
@windmaomao I'm afraid there are some concurrent issues that are very hard to resolve |
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you! |
propagateContextChange
in the current context implementation visits all child fibers rather than fibers that have dependency on the context.In my case, there may be thousands of child components and 99% of them don't have dependency on the context, which causes
propagateContextChange
spends a lot of scripting time but eventually does 'nothing'.The text was updated successfully, but these errors were encountered: