setting .textContent to the same value it currently has causes invalidation

NEW
Assigned to

Status

()

Core
Layout
5 years ago
2 months ago

People

(Reporter: gal, Assigned: smaug)

Tracking

({perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
This makes performance suck when web content animates something and carelessly updates the value over and over at every animation frame.
(Reporter)

Comment 1

5 years ago
Created attachment 595333 [details]
enable paint flashing when looking at this test case
(Reporter)

Updated

5 years ago
Blocks: 715782
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.
OS: Mac OS X → All
Hardware: x86 → All
(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.
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.
Assignee: nobody → roc
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.
Attachment #595352 - Flags: review?(jst)
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).
Attachment #595353 - Flags: review?(jst)
Attachment #595353 - Flags: feedback?(Ms2ger)
(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.
This seems somewhat fishy... Could you send email to www-dom to hear what other vendors think?
Does this mean I have to subscribe to yet another mailing list?
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?
Anne emailed:

http://lists.w3.org/Archives/Public/www-dom/2012JanMar/0038.html
(Assignee)

Comment 12

5 years ago
I thought webkit's innerHTML behavior was fixed when they moved to HTML5 parser.
But I could be wrong.
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.
Attachment #595352 - Flags: review?(jst) → review+
(Assignee)

Comment 14

5 years ago
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.
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.
(Assignee)

Comment 16

5 years ago
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.
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.
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.
Assignee: roc → nobody
(Assignee)

Comment 19

5 years ago
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.
Assignee: nobody → bugs
(Assignee)

Comment 20

5 years ago
Created attachment 595712 [details] [diff] [review]
WIP (to replace part 2)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f81109133b5
(Don't close bug when landing on central.)
(In reply to Olli Pettay [:smaug] from comment #20)
> Created attachment 595712 [details] [diff] [review]
> WIP (to replace part 2)

That is clever!
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.)
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.)
(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

5 years ago
See also bug 404385, same thing for setting innerHTML.

Updated

5 years ago
Attachment #595353 - Flags: feedback?(Ms2ger) → feedback-

Updated

5 years ago
Blocks: 715784

Updated

5 years ago
No longer blocks: 715782
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.
Attachment #595353 - Flags: review?(jst)

Updated

4 years ago
Blocks: 873425
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.
Keywords: perf
(Assignee)

Comment 29

4 years ago
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
Attachment #595712 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
(Assignee)

Comment 30

4 years ago
Created attachment 785129 [details] [diff] [review]
WIP
Attachment #785107 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Comment on attachment 785129 [details] [diff] [review]
WIP

Still broken.
(Assignee)

Comment 32

4 years ago
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.
Blocks: 921808
(Assignee)

Comment 33

4 years ago
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.
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?
(Assignee)

Comment 35

3 years ago
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).
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.
(Assignee)

Comment 37

3 years ago
MutationObserver API explicitly requires creating MutationRecords even if the attribute value
hasn't changed.
You need to log in before you can comment on or make changes to this bug.