ReactDOMComponentTree #5190

Closed
wants to merge 10 commits into
from

Projects

None yet

6 participants

@spicyj
Member
spicyj commented Oct 15, 2015

New module! With this, we have a new module that uses the component tree to store DOM nodes. Most of the logic in this file relates to markup adoption -- if we were to drop that (along with server rendering) this file could just be a return inst._nativeNode;.

This works with useCreateElement only because we need to reference each node as it's created.

Events is now the only thing using hierarchical IDs and ReactMount.getNode -- I'll use ReactDOMComponentTree.getInstanceFromNode to make that work.

@gaearon gaearon and 1 other commented on an outdated diff Oct 15, 2015
...ared/reconciler/__tests__/ReactEmptyComponent-test.js
@@ -81,9 +85,7 @@ describe('ReactEmptyComponent', function() {
);
});
- it('should be able to switch between rendering null and a normal tag', () => {
- spyOn(console, 'log');
-
+ it.only('should be able to switch between rendering null and a normal tag', () => {
@gaearon
gaearon Oct 15, 2015 Member

it.only

@spicyj
spicyj Oct 16, 2015 Member

Thanks, fixed.

@facebook-github-bot

@spicyj updated the pull request.

@facebook-github-bot

@spicyj updated the pull request.

@facebook-github-bot

@spicyj updated the pull request.

@gaearon
Member
gaearon commented Oct 16, 2015

What is the big picture here? Getting rid of data-reactid eventually? Better resilience against evil browser plugins?

@spicyj
Member
spicyj commented Oct 16, 2015

Getting rid of hierarchical IDs (replacing them with numbers, probably) and the giant hash map we have in ReactMount. We can probably kill data-reactid entirely when not using server-side rendering.

@jimfb
Contributor
jimfb commented Oct 16, 2015

@spicyj My hope is that we can kill data-reactid entirely especially when using server-side rendering. Rehydrating a markup tree does not benefit from having data-reactid afik.

@sebmarkbage sebmarkbage commented on the diff Oct 16, 2015
...derers/dom/shared/__tests__/ReactDOMComponent-test.js
@@ -52,7 +52,7 @@ describe('ReactDOMComponent', function() {
var stubStyle = container.firstChild.style;
// set initial style
- var setup = {display: 'block', left: '1', top: 2, fontFamily: 'Arial'};
+ var setup = {display: 'block', left: '1px', top: 2, fontFamily: 'Arial'};
@sebmarkbage
sebmarkbage Oct 16, 2015 Member

What's this about? Behavior change?

@jimfb
jimfb Oct 16, 2015 Contributor

Styles may be unitless if they are integers, but strings must specify units if required (unitless strings may fire a warning). This looks fine to me.

@spicyj
spicyj Oct 16, 2015 Member

@jimfb recently merged a PR that adds a warning for this case but it only checked the create case, not the update case which we use for createElement. I fixed that so the test for the warning passes with useCreateElement and fixed this test to not warn.

@spicyj
Member
spicyj commented Oct 16, 2015

@jimfb I'm operating under the assumption that browser extensions will insert extra nodes into our markup and we need to skip over them.

@sebmarkbage
Member

Make sure you do your react-native homework

@syranide
Contributor

My hope is that we can kill data-reactid entirely especially when using server-side rendering. Rehydrating a markup tree does not benefit from having data-reactid afik.

@jimfb FYI, we can already kill data-reactid for server-rendered markup, it's rather trivial as long as you're ok with doing a fast pass over all the nodes once when starting the client.

@facebook-github-bot

@spicyj updated the pull request.

@spicyj
Member
spicyj commented Oct 16, 2015

We can do that trivially here too by changing precacheChildNodes to not look at IDs and just match up the nodes – but I'm worried about finding extra nodes.

@syranide
Contributor

@spicyj We can always mark the nodes with just data-react, but leave out the ID. That should give you most of the benefits, while also tolerating foreign nodes. Personally I think we should just be super strict and simply not allow unexpected nodes, they are crashes waiting to happen.

@spicyj
Member
spicyj commented Oct 16, 2015

That's true, but I don't think including a numeric ID (data-reactid="17") is much overhead over that.

@jimfb
Contributor
jimfb commented Oct 16, 2015

Personally I think we should just be super strict and simply not allow unexpected nodes, they are crashes waiting to happen.

I tend to agree - if we find an unexpected node while restoring a SSR DOM tree, we can just kill it with fire. We don't even need a flag on the nodes to do that.

@syranide
Contributor

That's true, but I don't think including a numeric ID (data-reactid="17") is much overhead over that.

@spicyj data-react compresses trivially to virtually nothing, but yeah the overhead of the ID should be minimal too. Both are miles better than the current reactids anyway.

@facebook-github-bot

@spicyj updated the pull request.

@spicyj
Member
spicyj commented Oct 18, 2015

@jimfb @sebmarkbage The last two commits (edit: more by the time you read this, start with the one called "Events:") add events support so we don't rely on hierarchical IDs for events either. (Sorry for piling it all into the same PR – but github sucks at stacking PRs and I didn't want to merge this to master quite yet.) Either of you want to take a look? I'm a little nervous but all the FB test flows worked the first time with this applied.

@facebook-github-bot

@spicyj updated the pull request.

@spicyj
Member
spicyj commented Oct 18, 2015

Did a quick benchmark of rendering a PE-like hierarchy with Electron (Chrome) with a cold and warm JIT using my perf-counters as the timestamp.

With a warm JIT (rendering the hierarchy 100 times before measuring anything):

This PR is 3.3% faster than master.
Switching to numerical IDs (will be a separate PR, not quite done yet) brings us to 7.9% faster than master.
Numerical IDs with createElement instead of HTML generation is on par (also 7.9%).
Numerical IDs with createElement and not setting data-reactid at all (since we don't need it) gives another 1.4% boost for a total of 9.1%.

With a cold JIT, createElement looks a little bit slower (but this PR helps more compared to master!).

Our deltas of
[3.3%, 7.9%, 7.9%, 9.1%] with a warm JIT are
[4.9%, 9.6%, 8.3%, 8.9%] with a cold JIT.

Not sure if there are things we can do to improve the speed of createElement.

@syranide
Contributor

@spicyj Just for context, I assume those are React rendering times + innerHTML but do not include browser rendering/painting? PS. Also that's awesome :)

@spicyj
Member
spicyj commented Oct 18, 2015
var a = now();
React.render(React.createElement(Benchmark), container);
container.offsetHeight;
var b = now();

There's probably some painting involved. I intentionally forced layout: I wanted to make sure all the markup was parsed to make sure the HTML mode wasn't doing stuff lazily. Any ideas on how to keep that guarantee while avoiding painting (and layout)?

@syranide
Contributor

@spicyj I'd assume that would just force layouting, realistically rendering/painting should only happen after JS execution ends. I'm pretty sure innerHTML is not deferred as you can measure differences in time by the number of tags/attributes/styles/etc present. It probably doesn't really make sense to defer it in practice either I would think.

@spicyj
Member
spicyj commented Oct 18, 2015

You're probably right. I tried it again without the forced layout and it's about 24ms (on master: 210ms vs 186ms) faster on all five numbers, so the percentages are proportionally the same but go up a little because of a smaller base. Cold JIT, just React.render (no layout): [5.5%, 10.8%, 9.2%, 10.0%].

@spicyj
Member
spicyj commented Oct 18, 2015

Numerical IDs are in #5205. Also, devtools don't work with this because they currently rely on ReactMount.getNode.

spicyj added some commits Oct 15, 2015
@spicyj spicyj Make ReactEmptyComponent-test not swallow logs 19e2cf5
@spicyj spicyj Remove obsolete tests with new tree-walking 5becbe3
@spicyj spicyj Make ReactDOMComponent-test pass with useCreateElement 276ef73
@spicyj spicyj Always do useCreateElement for updates
Initial render can still be a markup string.
5a7c273
@spicyj spicyj ReactDOMComponentTree
New module! With this, we have a new module that uses the component tree to store DOM nodes. Most of the logic in this file relates to markup adoption -- if we were to drop that (along with server rendering) this file could just be a `return inst._nativeNode;`.

This works with useCreateElement only because we need to reference each node as it's created.

Events is now the only thing using ReactMount.getNode -- I'll introduce pointers back from the DOM nodes (and a `ReactDOMComponentTree.getInstanceFromNode`) and make that work.
ce52845
@spicyj spicyj Add functions to find the instance given a node 6d20556
@spicyj spicyj Events: Pass native instances up until propagation
Next step: take advantage of having the native instances in EventPropagators instead of converting right back to IDs.
d946dee
@spicyj spicyj Propagate events based on component tree, not IDs 5e83125
@spicyj spicyj put/deleteListener take an instance, not an ID
This removes SimpleEventPlugin's dependency on ReactMount.getID.
9d93af2
@spicyj spicyj Kill ReactMount.getNode/getID/purgeID with fire c85e591
@facebook-github-bot

@spicyj updated the pull request.

@spicyj
Member
spicyj commented Nov 4, 2015

Merged in #5205.

@spicyj spicyj closed this Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment