As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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] (review request backlog because of a work week)
:
: 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] (review request backlog because of a work week)
no flags Details | Diff | Splinter Review
up-to-date WIP (8.05 KB, patch)
2013-08-02 11:29 PDT, Olli Pettay [:smaug] (review request backlog because of a work week)
no flags Details | Diff | Splinter Review
WIP (8.08 KB, patch)
2013-08-02 12:03 PDT, Olli Pettay [:smaug] (review request backlog because of a work week)
no flags Details | Diff | Splinter Review

Description User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image :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 User image 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 User image 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 User image :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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2012-02-09 05:53:43 PST
Created attachment 595712 [details] [diff] [review]
WIP (to replace part 2)
Comment 21 User image 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 User image 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 User image 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 User image 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 User image 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 User image Jesse Ruderman 2012-02-17 18:03:26 PST
See also bug 404385, same thing for setting innerHTML.
Comment 27 User image 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2013-08-02 12:03:10 PDT
Created attachment 785129 [details] [diff] [review]
WIP
Comment 31 User image Olli Pettay [:smaug] (review request backlog because of a work week) 2013-08-02 12:10:17 PDT
Comment on attachment 785129 [details] [diff] [review]
WIP

Still broken.
Comment 32 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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 User image 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 User image Olli Pettay [:smaug] (review request backlog because of a work week) 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.