Skip to content

Proof-of-concept, remove data-reactid and nodeCache, traverse inserted markup immediately #1550

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

Closed
wants to merge 6 commits into from

Conversation

syranide
Copy link
Contributor

Proof-of-concept implementation complete:

There are some ugly hacks (for simplicity at this point), I mainly provide this PR as a topic for discussion, I intend to rewrite it properly and from scratch, if a consensus is reached.

  • data-reactid has been removed from the DOM! Immediately after rendering of markup, it now traverses the DOM nodes and ReactComponents in sync.
  • DOM nodes now have __reactComponent__ property which holds the ReactDOMComponent instance.
  • ReactComponents now have _rootNode which holds the DOM node.
  • _rootNodeID still exists but could quite easily be replaced with a monotonic ID (I'm not even sure we need that, but might be useful).
  • nodeCache has been completely removed, no cache to maintain! Additionally, apart from old browsers possibly leaking memory (IE8/IE9 perhaps?), it is no longer strictly necessary to unmount React from a node I believe, but you really should.
  • Listeners are now registered directly on the ReactDOMComponent (we could probably avoid explicitly registering too).
  • All the possibilities! It's even imaginable that we can unboxing/split ReactTextComponent cheaply during traversal! Perhaps it's even more efficient to set the style-object directly as opposed to generating and escaping it.
  • I may be reaching to foregone conclusions here, but I believe this also opens up the possibility to have true empty/multi-child components without taking on a significant performance-hit (or rather, brings us closer to).
  • Due to immediately traversing the DOM nodes (in sync with React Components), we can choose to either error immediately if the browsers mucked too much with the DOM, or try to solve when possible (for say tables) although solving correctly may be tricky.

The biggest issue moving forward is tests (if you agree with my findings). This PR breaks pretty much every single test there is (either by API change/deprecation or because they're no longer relevant), it's very likely that we want to leave shims in-place which should allow us to avoid most of the issues for now though.

This PR does not support server-rendering (I think), but it is trivial to fix. We can also get rid of rootIndex.


Results:
VMIE8: http://i.imgur.com/EPlzmeq.png, http://i.imgur.com/92UdT7K.png
VMIE9: http://i.imgur.com/Q9ya2RY.png, http://i.imgur.com/cYpbxtf.png
VMIE10: http://i.imgur.com/Oh0pB1B.png, http://i.imgur.com/FJJtVvB.png
IE11: http://i.imgur.com/toWaPsS.png
FF: http://i.imgur.com/tmy9UT1.png
Chrome: http://i.imgur.com/m3EMucK.png

The IE8 test is especially interesting because of IE8's simplicity, you can see a significant and consistent performance increase in basically every test.

   raw     gz Compared to master @ 46de927a28eb788b4329589658dbd31a014e4edd
 -2617   -849 build/react-test.js
 -5911  -1607 build/react-with-addons.js
 -1010   -415 build/react-with-addons.min.js
 -5911  -1601 build/react.js
 -1009   -412 build/react.min.js

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@syranide
Copy link
Contributor Author

cc @sebmarkbage @petehunt @zpao @spicyj

I would like your criticism on this PR (but not the code, it's poor).

In my mind, unless I missed something, I'm confident that this is the way forward even taking a small perf hit (but it seems, that overall, the opposite is true) would be worth all the advantages that comes as a result. Do you guys agree?

I'm assuming the preferred way forward (if accepted), would be to do it incrementally:

  1. Immediately traverse and prime nodeCache.
  2. Deprecate data-reactid, store React ID + Component on DOM node, store DOM node on React Component.
  3. Deprecate use of React ID for events, store listeners on React Component or DOM node instead.
  4. Deprecate nodeCache, React ID and related APIs.
  5. Revise internal API if necessary, remove putEventListener, etc, in favor of querying React DOM Component directly?

@syranide syranide changed the title Immediately prime node cache with inserted elements, rather than lazily Remove data-reactid and nodeCache, traverse inserted markup immediately May 18, 2014
@syranide syranide changed the title Remove data-reactid and nodeCache, traverse inserted markup immediately Proof-of-concept, remove data-reactid and nodeCache, traverse inserted markup immediately May 18, 2014
@syranide
Copy link
Contributor Author

One part of the implementation available at #1570

@syranide
Copy link
Contributor Author

Closing in-favor of #1570 and additional PRs, graphs and "intended end-result" may still be of interest in this PR.

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

Successfully merging this pull request may close these issues.

2 participants