Consider special-casing innerHTML set when only one textnode child before and after

RESOLVED INACTIVE

Status

()

Core
DOM
P4
normal
RESOLVED INACTIVE
11 years ago
a day ago

People

(Reporter: bz, Assigned: smaug)

Tracking

({perf})

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

When we set innerHTML on a node and:
1)  The node only has a single textnode child
2)  The document fragment returned by createContextualFragment only has a single
    textnode child

we should consider just setting the existing textnode's text.  This is what Safari does.  This would allow us to reduce frame construction costs, etc.

In fact, we could even drop condition 1 if we use our SetNodeTextContent thing.  

The only drawback is that this violates the proposed WHATWG innerHTML spec, as far as sicking and I can tell.
(Assignee)

Comment 1

11 years ago
That change would affect mutation events, and in fact I think there is some existing code which relies on mutation events even in that case, see bug 388563 :(
But ofc we could take the optimized path if there are no mutation event listeners.
(Assignee)

Comment 2

11 years ago
Would it be difficult or impossible to reuse the textframe, but still update
DOM properly?

Comment 3

11 years ago
Nominating for blocking since this is a performance spinoff of 386769 
Flags: blocking1.9?

Comment 4

11 years ago
Seems like an odd thing to do -- I wouldn't expect the text node to be reused like that.

How much would this help perf?

Comment 5

11 years ago
(In reply to comment #4)
> Seems like an odd thing to do -- I wouldn't expect the text node to be reused
> like that.
> 
> How much would this help perf?
> 

Dunno how much the patch would help but checked the blocked bug for perf comparisons - we have quite a ways to catch up.
What we should do here is change the spec to say that mutating the existing node in this case is the right thing to do. I'm half-way through an email about this to the whatwg group.
> That change would affect mutation events

True.  We'd still fire DOMCharacterDataModified, of course....

> Would it be difficult or impossible to reuse the textframe, but still update
> DOM properly?

Impossible, no.  Difficult, yes.  You'd need to swap out the content pointer in the existing frame, somehow.

> How much would this help perf?

Hard to say for sure, but right now of the 137032 total hits, we spend 12657 constructing the new textframe and 6789 removing the old frame.  That's about 14% of total time.  If I look at the frame-manipulation as a fraction of the time actually under nsGenericHTMLElement::SetInnerHTML, it's 21% of that time.  This wouldn't go away completely, of course; I wouldn't bet on more than a 10% win on overall time.
Assignee: nobody → Olli.Pettay
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(Assignee)

Comment 8

11 years ago
Created attachment 289671 [details] [diff] [review]
for testing, reuse the domnode

IMHO, it is a bug in Webkit that it reuses the DOMNode.
If someone wants to reuse, element.firstChild.data = "some text" can be used.
What we should do here is to reuse the textframe.
(Assignee)

Comment 9

11 years ago
I tested the patch only on a debug build and there 
https://bugzilla.mozilla.org/attachment.cgi?id=270828 dropped from
~400 to ~300
Please repeat after me:

  PERFORMANCE TESTING IN DEBUG BUILDS IS USELESS.  IT LIES.  A LOT.
I do think we should do what webkit does, but only after getting the spec changed to say that that is the right thing to do.

Setting .innerHTML is something that a lot of people are doing, i doubt we can educate them to say that they should use .firstChild.data instead.

I also can't see much reason *not* to reuse the textnode.
(Assignee)

Comment 12

11 years ago
(In reply to comment #11)
> I also can't see much reason *not* to reuse the textnode.

One reason is to keep .innerHTML functionality closer to .textContent, which we can't change because DOM 3 Core has been a recommendation for ages.
And to make .innerHTML work in a different way depending on the value
it is assigned is kind of weird. Consistency is important.
(Assignee)

Comment 13

11 years ago
Created attachment 289737 [details] [diff] [review]
reuse textframes, an ugly patch for testing

On debug build this has almost similar speed up as reusing domnode.
Need to still test these with opt build.
But anyway, this shows that reusing textframes might not be that difficult -
ugly, but not difficult. (I may have missed something though)
That code breaks with ::first-letter, I believe.
(Assignee)

Comment 15

11 years ago
ah, probably.
(Assignee)

Comment 16

11 years ago
But looking at nsCSSFrameConstructor::CharacterDataChanged, shouldn't
be difficult to fix.
(Assignee)

Comment 17

11 years ago
Created attachment 289752 [details] [diff] [review]
reuse textframes, not so ugly.

Use the normal code path when changing chardata.
Attachment #289737 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
Created attachment 289857 [details]
testcase
(Assignee)

Comment 19

11 years ago
On my machine current opt trunk (-O3) avg 82, with textframe reuse avg 68, textnode reuse avg 63.
(Assignee)

Comment 20

11 years ago
But textframe patch doesn't quite work yet with long texts. Need to clean up something I guess.
(Assignee)

Comment 21

11 years ago
Created attachment 291423 [details]
testcase2, long text, :first-letter and :first-line
(Assignee)

Comment 22

11 years ago
Created attachment 291426 [details] [diff] [review]
works with long text
(Assignee)

Comment 23

11 years ago
Opt Linux 32bit build:
testcase:  avg ~90  with patch, 103 without patch
testcase2: avg ~128 with patch, 144 without patch
(Assignee)

Comment 24

11 years ago
Roc, what do you think about re-using textframe? Is there some case the patch
doesn't handle?
Ugly optimization but reusing DOMNodes would be just wrong, IMHO
(because of spec, .textContent, event handlers on textnode etc.).
I think it would be considerably cleaner to reuse the textnode, and eventually more efficient (e.g. by optimizing the parser to not create a new node). We just don't change the content under a frame anywhere else and I'm afraid this will make things fragile.

Can you have event handlers on textnodes? We can detect that anyway. What's the problem with .textContext? We can get the spec changed and anyway, in 1.9, I think we could check the refcount of the node to see if anyone's holding a reference to it other than the DOM tree (that wouldn't work with XPCOMGC though).
As i said before, i think we should get the spec changed here, i think that's a win for everyone, our code will be cleaner, the perf win will be bigger, and i think it's what authors would find more useful if they did in fact have event listeners on the textnode.
(Assignee)

Comment 27

11 years ago
Ofc textnodes can have event listeners. All nodes can have listeners, even
attribute nodes.

.textContent is from DOM Level 3, which has been recommendation for ages, and
it clearly says that new node should be created. It would be strange if 
.innerHTML behaved differently. Currently HTML5 says that new node
should be created when .innerHTML is used. What WebKit does is wrong.
And to repeat myself, making .innerHTML have one special case... doesn't sound 
good.

How could I check refcounter value? Parent addrefs, all textframes addref.
Though perhaps counting all textframes + parent is not too ugly, 
and also check that there are no mutation event listeners, and no event 
listeners added to textnode, ...what else...
(Assignee)

Comment 28

11 years ago
...no DOM 3 userData...
I think trying to figure out when a node is unused is a loosing battle. Any random change to any of the features we have in the tree could change the number of references held to the node. And we probably already have features that hold varying number of references, text that wraps over two lines will have two frames, right?

Oh, and with MMgc refcounting is going away entirely...
> Oh, and with MMgc refcounting is going away entirely...

Right, that's what I said in comment #25.
With JS we're more or less living in that world already. As soon as scripts touch the node there is going to be a reference to it from the wrapper that will go away at a more or less random point (i.e. when GC is run)
That's actually fine because 99.9% of the time JS will never have touched this text node.

With MMgc/XPCOMGC, testing the refcount doesn't work at all, because there isn't one. So the situations are pretty different.
(Assignee)

Comment 33

11 years ago
So what do we want to do here?
I think there are three options:
(1) do what the patch does - reuse textframe
(2) try to figure out if textnode is owned only by parent and relevant
    textframes and there are no event listeners nor DOM 3 userData handlers etc.
    and if everything looks good, reuse domtextnode. Won't work after 1.9.
(3) do nothing specific to this bug and try to optimize other parts;
    frame construction, parsing etc. Possibly too late for 1.9.

What WebKit does is just plain wrong and not compatible what IE does.
It's only "plain wrong" because the spec says so IMHO. So I'd add

(4) try to get the spec changed and then reuse the textnode.

Which is what I think we should do.
(Assignee)

Comment 35

11 years ago
But IE doesn't reuse textnode (and repeating myself, feels *very* strange to
special case something like this where the only somewhat good reasoning is to
ease fast-working implementation)

Comment 36

11 years ago
So - do we want to try and do this for 1.9 or not?

Comment 37

11 years ago
No motion - moving off blocking list for now.  Re-nom if you disagree
Flags: blocking1.9+ → blocking1.9-

Updated

10 years ago
No longer blocks: 386769

Updated

10 years ago
Blocks: 442348

Comment 38

6 years ago
See also bug 725221, same thing for setting textContent.
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core

Comment 39

4 years ago
Nightly
testcase - total: 1570, avg 3.14
testecase2 - total: 288, avg 19.2

Chrome 32
testcase - total: 2970, avg 5.94
testcase2 - total: 263, avg 17.533333333333335

IE 11
testcase - total: 7082, avg 14.164
testcase2 - total: 377, avg 25.133333333333333
(Assignee)

Comment 40

4 years ago
This was partially fixed in bug 922681, and bug 959150 will help in case of long text.

Comment 41

a day ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: a day ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.