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

[performance] Inline ReactChildReconciler.instantiateChildren #3922

Closed
wants to merge 1 commit into from

Conversation

mridgway
Copy link
Contributor

Follow up from #3717

if (__DEV__) {
warning(
true,
'mountChildren(...): Encountered two children with the same key, ' +
Copy link
Collaborator

Choose a reason for hiding this comment

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

(instantiateChildren)

@sophiebits
Copy link
Collaborator

Code looks fine. Will try to replicate your perf-test results and if it looks positive I'll merge.

@mridgway
Copy link
Contributor Author

Updated and rebased.

@sophiebits
Copy link
Collaborator

@jimfb Assigning to you.

@jimfb
Copy link
Contributor

jimfb commented Jul 18, 2015

Yep, this diff is freakishly similar to my diff. We decided to merge #4408, since I was already iterating on it with Sebastian. For this reason, I'm closing out this diff.

@mridgway Your diff was spot on. I truly wish we had seen it earlier. If we had merged this back in May, it would have saved us a whole bunch of time/energy :P. Ah well, such is life. We didn't know then what we know now (didn't have the perf tests on our end). If you have any other ideas for improving perf and/or otherwise improving React, please do ping us; we can do a video chat or grab lunch or whatever.

@jimfb jimfb closed this Jul 18, 2015
@sophiebits
Copy link
Collaborator

@mridgway Sorry for dropping the ball on this one. Got caught up in preparing for the conference and then I was out on vacation for a couple weeks. I could've passed this to @jimfb when he started focusing on perf work but didn't think of it at the time. Please do let us know if you find more improvements and hopefully we can get them merged sooner next time.

@mridgway
Copy link
Contributor Author

No worries. I'm just glad someone is looking into performance 👍.

@jimfb I may take you up on lunch sometime. I'm eager to work with you guys on optimizing the server rendering flow but need some more information on what the plan is before I can contribute. Looks like #4012 has some more details that I'll look into.

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

Successfully merging this pull request may close these issues.

None yet

5 participants