Last Comment Bug 725221 - setting .textContent to the same value it currently has causes invalidation
: setting .textContent to the same value it currently has causes invalidation
Status: NEW
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Jet Villegas (:jet)
Mentors:
http://mozilla.pettay.fi/moztests/tex...
Depends on:
Blocks: 873425 b2g-product-phone 921808
  Show dependency treegraph
 
Reported: 2012-02-08 00:13 PST by Andreas Gal :gal
Modified: 2014-10-02 02:27 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
enable paint flashing when looking at this test case (264 bytes, text/html)
2012-02-08 00:14 PST, Andreas Gal :gal
no flags Details
part 1: don't fire notifications when text doesn't change (2.71 KB, patch)
2012-02-08 01:57 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jst: review+
Details | Diff | Splinter Review
Part 2: reuse first text node child when setting textContent (858 bytes, patch)
2012-02-08 02:01 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
Ms2ger: feedback-
Details | Diff | Splinter Review
WIP (to replace part 2) (2.42 KB, patch)
2012-02-09 05:53 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
up-to-date WIP (8.05 KB, patch)
2013-08-02 11:29 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
WIP (8.08 KB, patch)
2013-08-02 12:03 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review

Description Andreas Gal :gal 2012-02-08 00:13:58 PST
This makes performance suck when web content animates something and carelessly updates the value over and over at every animation frame.
Comment 1 Andreas Gal :gal 2012-02-08 00:14:45 PST
Created attachment 595333 [details]
enable paint flashing when looking at this test case
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 01:07:01 PST
This is actually a pain since the DOM4 spec calls for creating a new text node and replacing the contents of the element with it. And that requires frame reconstruction. So the simplest way to fix this would be to violate/change the DOM4 spec.

We could change the spec for setting Element/DocumentFragment textContent so that if it has a single text node child already, defer to setting textContent on the text node. Anne, what do you think?

Then we could implement that, and also optimize nsDOMGenericDataNode::SetTextInternal to do nothing when the new text equals the replaced text.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 01:25:21 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> We could change the spec for setting Element/DocumentFragment textContent so
> that if it has a single text node child already, defer to setting
> textContent on the text node. Anne, what do you think?

... if the new text is a non-empty string.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 01:30:55 PST
Or we could say that if the first child is a text node, then we set the textContent of the text node and remove the rest of the children. That matches the aTryReuse option in nsContentUtils::SetNodeTextContent.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 01:57:30 PST
Created attachment 595352 [details] [diff] [review]
part 1: don't fire notifications when text doesn't change

This changes mutation event behavior --- events don't fire when the old and new text is the same --- but I'm pretty sure we don't care about that.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 02:01:58 PST
Created attachment 595353 [details] [diff] [review]
Part 2: reuse first text node child when setting textContent

This violates the current DOM4 spec. I think we should just change the spec to allow/require this.

The exact behavior change is, when setting textContent on an element or document fragment whose first child is a text node, we do the equivalent of setting .data on that text node and removing the rest of the children (instead of removing all the children and creating a new text node).
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 02:12:12 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> The exact behavior change is, when setting textContent on an element or
> document fragment whose first child is a text node,

"... and the new text is non-empty,"

Actually I have a nagging suspicion that someone might have tried this in the past and backed off for some reason. But maybe I'm imagining it.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-02-08 02:44:00 PST
This seems somewhat fishy... Could you send email to www-dom to hear what other vendors think?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 02:46:50 PST
Does this mean I have to subscribe to yet another mailing list?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-02-08 06:11:12 PST
This sounds like the optimization that WebKit does for innerHTML in some cases...  I can't recall what the spec on that ended up being.

Does WebKit already do this for textContent too?
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-02-08 08:04:49 PST
Anne emailed:

http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0038.html
Comment 12 Olli Pettay [:smaug] 2012-02-08 09:49:56 PST
I thought webkit's innerHTML behavior was fixed when they moved to HTML5 parser.
But I could be wrong.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-08 09:59:11 PST
Comment on attachment 595352 [details] [diff] [review]
part 1: don't fire notifications when text doesn't change

Yeah, agreed, we should at least take this optimization.
Comment 14 Olli Pettay [:smaug] 2012-02-08 16:11:24 PST
So, did anyone profile .textContent handling?
Is the the creation of new frame which takes time or what?

I'd prefer if we could reuse the textframe, not dom node.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 16:24:34 PST
The problem is the invalidation and painting, not the creation of frames and nodes.

So to follow the spec and still get the performance win, we'd need to switch the node out from under the frame without informing the frame that anything's changed. Maybe it could be done, but changing the content for a frame violates a fundamental layout invariant. It's much cleaner (and ultimately, more efficient) to reuse the text node.
Comment 16 Olli Pettay [:smaug] 2012-02-08 16:30:03 PST
Reusing text node breaks the API which has been there for almost a decade.
I'd rather make changes to Gecko internal APIs than web APIs.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 17:28:51 PST
We're talking about breaking mutation events, which are just as old and which we know will break some code (unlike here).

Preserving compatibility does not automatically trump any amount of implementation difficulty.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-08 17:34:39 PST
Anyway, I don't have the energy to deal with this. Andreas, if you need this, you'd better find someone on the content team to pick it up.
Comment 19 Olli Pettay [:smaug] 2012-02-09 03:34:21 PST
I can look at this soonish.
We could either do some layout hackery, or optimize the hopefully common case when
only parent node and nsIFrame own the text node and there are no mutation event listeners
or (DOM) mutation observers.
Comment 20 Olli Pettay [:smaug] 2012-02-09 05:53:43 PST
Created attachment 595712 [details] [diff] [review]
WIP (to replace part 2)
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-13 21:00:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f81109133b5
(Don't close bug when landing on central.)
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-13 21:02:37 PST
(In reply to Olli Pettay [:smaug] from comment #20)
> Created attachment 595712 [details] [diff] [review]
> WIP (to replace part 2)

That is clever!
Comment 23 Phil Ringnalda (:philor) 2012-02-13 22:37:31 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7f81109133b5 for a fistful of orange in content/base/test/test_bug415860.html - your guess is better than mine about which one in the push was at fault.

(Rather than a comment that can wind up a ways up from the bottom, to get a bug left open, put "[Leave open after merge]" in the whiteboard.)
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-14 00:57:23 PST
It was this patch.

The test does

  n.textContent = "Hello!";
  r = document.createRange();
  r.setStart(n, 4);
  r.setEnd(n, 5);
  s.addRange(r);
  ...
  n.textContent = "Hello!";
  ok(s == "", "Should have selected ''");

The test expects setting the text node to anything at all to collapse the selection, which is actually what the spec wants. So we need to do something else here, like add a boolean to CharacterDataChangeInfo to indicate whether the text actually changed. (It's a bit of a pain since most of the CharacterDataWillChange/CharacterDataDidChange listeners don't need to be notified if the text actually hasn't changed.)
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-14 00:59:39 PST
(In reply to Olli Pettay [:smaug] from comment #20)
> Created attachment 595712 [details] [diff] [review]
> WIP (to replace part 2)

BTW if you're going to do this, you probably should count the frames starting with the primary frame and following nsLayoutUtils::GetFirstContinuationOrSpecialSibling. You don't want the optimization to be defeated by bidi or line breaking.
Comment 26 Jesse Ruderman 2012-02-17 18:03:26 PST
See also bug 404385, same thing for setting innerHTML.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2012-11-12 23:09:07 PST
Comment on attachment 595353 [details] [diff] [review]
Part 2: reuse first text node child when setting textContent

Clearing review request here. I think Olli's approach is probably the better way forward here.
Comment 28 Jonathan Watt [:jwatt] 2013-05-18 18:10:04 PDT
Olli, any progress on this?

It seems that D3.js ( http://d3js.org/ ) does this a lot (or makes it easy for users to do it a lot), and is hurting us relative to other implementations - see bug 873425.
Comment 29 Olli Pettay [:smaug] 2013-08-02 11:29:04 PDT
Created attachment 785107 [details] [diff] [review]
up-to-date WIP

https://tbpl.mozilla.org/?tree=Try&rev=8aebb68352f8

Would be still nice to find some real world testcase
Comment 30 Olli Pettay [:smaug] 2013-08-02 12:03:10 PDT
Created attachment 785129 [details] [diff] [review]
WIP
Comment 31 Olli Pettay [:smaug] 2013-08-02 12:10:17 PDT
Comment on attachment 785129 [details] [diff] [review]
WIP

Still broken.
Comment 32 Olli Pettay [:smaug] 2013-08-02 12:27:13 PDT
Hmm, this gets just too tricky. FormFillController, and its tests expects (correctly, per spec)
that setting .textContent removes the existing nodes and adds a new one.
Since FFC uses nsIMutationObserver, it becomes really tricky to optimize that case.
Comment 33 Olli Pettay [:smaug] 2013-10-22 15:35:05 PDT
So one way to fix this is to change nsIMutationObserver method signatures so that old node can
actually stay where it is, but if the new params to the methods aren't handled, it looks like
a node is removed and then a new one inserted.
Need to think that still through tough.
Comment 34 Zibi Braniecki [:gandalf][:zibi] 2014-08-11 14:32:53 PDT
This may cost us a lot on retranslations since we don't check if the textContent changed. Is there any hope to have the engine become smart about it?
Comment 35 Olli Pettay [:smaug] 2014-08-11 14:38:51 PDT
Doesn't translation use mutation observer? In that case we have to remove and
recreate the dom node (because that is the only sane thing to do from API perspective).
Comment 36 Zibi Braniecki [:gandalf][:zibi] 2014-08-11 15:00:31 PDT
It does, but gaia apps are not checking if the l10nId really changed when they reset data-l10n-id on elements. If MutationObserver is smart enough not to retrigger onMutations if the attributes value has not changed, then we're good here.

Otherwise, we retrigger l10n on the node and reset textContent to the same value.
Comment 37 Olli Pettay [:smaug] 2014-08-12 01:29:01 PDT
MutationObserver API explicitly requires creating MutationRecords even if the attribute value
hasn't changed.

Note You need to log in before you can comment on or make changes to this bug.