-
Notifications
You must be signed in to change notification settings - Fork 46.7k
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: setTextContent should attempt to set TextNode nodeValue where possible #7005
Conversation
… is a more performant text update operation. Also removed the IE8 polyfill for textContent (IE8 is no longer supported?).
…ntent gets called with same argument count
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 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 |
@spicyj You definitely want to avoid any calls to 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. :) |
|
@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 |
@trueadm updated the pull request. |
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 |
My curiosity has been piqued. I'm curious to see the impact, even on a synthetic benchmark. |
…nodeValue should be used in place of textContent
@trueadm updated the pull request. |
@trueadm updated the pull request. |
1 similar comment
@trueadm updated the pull request. |
Okay, I've made those changes. Here's a quick rundown on performance: http://infernojs.org/benchmarks/react/dbmonster/15/ - without The difference in performance, comparing both operations: http://infernojs.org/benchmarks/react/dbmonster/textContent.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. |
@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 |
@trueadm updated the pull request. |
@syranide Good point, I've added that just now. In terms of a quick test, IE9+ all look good to me. |
Works for me, thank you! |
As per title, this PR attempts to improve React's
TextNode
update/patch performance by accessing thefirstChild.nodeValue
. This can have a big impact on performance whereTextNode
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.