Performance: setTextContent should attempt to set TextNode nodeValue where possible

[trueadm]

As per title, this PR attempts to improve React's TextNode update/patch performance by accessing the firstChild.nodeValue. This can have a big impact on performance where TextNode updates are frequently changing.

This PR also removes the polyfill for element.textContent as this is no longer required for IE9+. No new tests were added to this PR as the test coverage already covers this use-case, let me know if you think this is wrong and I can add tests for use cases that may not have been covered given the changes.

[sophiebits]

Thanks for sending this in! Can you say what magnitude of performance gain this might give?

Instead of threading the boolean down, can we match the performance by checking if .childNodes.length === 1 and .firstChild.nodeType === 3? I'm also worried about supporting if browsers decide to split text nodes arbitrarily, which Firefox at least used to do every 4096 chars when parsing HTML.

Let's leave the IE8 workaround; we're not doing active work to support IE8 but aren't quite ready to break it intentionally yet. (Side note: I wonder if text node .nodeValue works correctly in IE8?)

[trueadm]

@spicyj You definitely want to avoid any calls to .childNodes as it's essentially a computed property, which creates a new live NodeList upon calling the function – which is very expensive. Ideally, if we can leverage the fact that we know the last child value for this given virtual node was a text value, we can thus assume we can access the firstNode.nodeValue to update the next value. On another note, accessing nodeType is cheap, although still not as cheap as accessing a property on a potentially monomorphic virtual node (where the hidden class is the same).

In terms of performance, depending on the scenario and application, you can see a big performance gain where an element's child text node constantly updates. If I were to provide a synthetic benchmark tomorrow of something like the infamous "dbmonster", before-and-after, would that be beneficial?

I'll add a commit to re-add the IE8 workaround later, once I get a chance to as well. :)

[sophiebits]

.firstChild === .lastChild then maybe? I see where you're coming from about the monomorphism but it still seems a little cumbersome to track that and I'm not sure I trust us to not screw it up.

[syranide]

I see where you're coming from about the monomorphism but it still seems a little cumbersome to track that and I'm not sure I trust us to not screw it up.

@spicyj We're trying to be somewhat lenient towards extensions altering the markup in minor ways and that would explicitly make it break in those cases.

I'm also really curious how big of a perf difference there really is here, I get that textContent may be more costly and that it would show up as significant in a pure textContent vs nodeValue-benchmark. But it seems odd that it would have a significant impact when considering a realistic example including all of the React overhead as well, but then again you never know :)

[ghost]

@trueadm updated the pull request.

[trueadm]

But it seems odd that it would have a significant impact when considering a realistic example including all of the React overhead as well, but then again you never know :)

It completely depends on the use-case. There are plenty of real-world use cases where a page may contain a table/grid of data values that frequently update (my company builds financial trading applications where we have a blotter of multiple fields that update every 250ms for example). If you have an application where nothing really updates and it's mostly static content, you won't see much gain.

If there's a demand, I can get some benchmarks in place to show the difference, i.e. dbmonster before-and-after.

Side note: I've re-added the IE8 work-around. @spicyj @syranide how do you want to proceed with determining if an element's child TextNode can be mutated directly via nodeValue rather than textContent?

[jimfb]

If there's a demand, I can get some benchmarks in place to show the difference, i.e. dbmonster before-and-after.

My curiosity has been piqued. I'm curious to see the impact, even on a synthetic benchmark.

[syranide]

@trueadm .firstChild === .lastChild as suggested by @spicyj should be bullet-proof (EDIT: and checking nodeType) as far as I'm aware. But please check whether nodeValue actually works in IE8-9 if you can, I have a vague memory of there being something funky with it.

[ghost]

@trueadm updated the pull request.

[ghost]

@trueadm updated the pull request.

[ghost]

@trueadm updated the pull request.

[trueadm]

Okay, I've made those changes. Here's a quick rundown on performance:

http://infernojs.org/benchmarks/react/dbmonster/15/ - without textContent
http://infernojs.org/benchmarks/react/dbmonster/15opt/ - with nodeValue

The difference in performance, comparing both operations:

http://infernojs.org/benchmarks/react/dbmonster/textContent.png
http://infernojs.org/benchmarks/react/dbmonster/nodeValue.png

In terms of the benchmark, I get about 2-3 FPS difference on this MacBook Pro I'm using. It's a big enough difference IMO to be worth consideration.

[syranide]

@trueadm It's hard to really quantify it, but I'm seeing a ~2% improvement here on Chrome and if you're seeing even more then it seems not without merit. Anyway, you should probably check nodeType as well, it may have been replaced with a node too.

[ghost]

@trueadm updated the pull request.

[trueadm]

@syranide Good point, I've added that just now. In terms of a quick test, IE9+ all look good to me.

[sophiebits]

Works for me, thank you!