-
Notifications
You must be signed in to change notification settings - Fork 48.7k
Removed flattened children object. Fixes #4405 #4408
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
Conversation
1b01044
to
d6c2373
Compare
@@ -25,6 +27,25 @@ var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); | |||
*/ | |||
var ReactChildReconciler = { | |||
|
|||
|
|||
_instantiateChild: function(childInstances, child, name) { |
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.
Let's move this to a module level function so that it is not exported. Ensures that it can't be overridden, minimizes better etc.
There is only one more caller in sliceChildren. It is going to need to be rewritten to something like #3650 anyway. To start with we can at least inline flattenChildren into sliceChildren and get rid of the entire module all together. |
d6c2373
to
fe99e59
Compare
@sebmarkbage I think there is still another caller in ReactChildReconciler, so it still needs to be its own module, right? @mridgway Wow, I wish I had seen that PR before I implemented literally the same thing :/. Do you have any other PRs that I should be aware of? :P. |
I see. We should fix the update phase too but that's a bit trickier so we can leave that to a follow up. |
@sebmarkbage Yeah, I've spent the last two hours reading through the update reconciliation code. I want to make a really aggressive change to the way we do updates. Specifically, I want to do the actual reconciliation by walking through lists instead of using a map/object. I'm pretty sure we can make the the common case (adding/deleting/updating nodes) substantially faster by making the worst case (reordering) a little bit slower (achieved by using an array instead of a object/map for instances). It may also involve inlining ReactChildReconciler.updateChildren. Unless you see any obvious show-stoppers to such an idea, I may take a stab at it this weekend and we can chat on Monday. |
Removed flattened children object. Fixes #4405
This is the most complex code in React. We've rewritten it multiple times (for example with linked lists) without gaining much better perf than this for our preferred cases, and there are literally whole vdom libraries dedicated to this piece code. So don't underestimate this task. Curious to see what algorithm you had in mind though and excited if it's faster/better. |
Removed flattened children object for initial render. Fixes #4405. Saves about 100ms (20% better TTI) for my large-list pest test :). Perf! Sweet Perf!