Closed
Bug 404385
Opened 17 years ago
Closed 2 years ago
Consider special-casing innerHTML set when only one textnode child before and after
Categories
(Core :: DOM: Core & HTML, defect, P4)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: bzbarsky, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(5 files, 1 obsolete file)
3.23 KB,
patch
|
Details | Diff | Splinter Review | |
13.03 KB,
patch
|
Details | Diff | Splinter Review | |
872 bytes,
text/html
|
Details | |
1.68 KB,
text/html
|
Details | |
14.21 KB,
patch
|
Details | Diff | Splinter Review |
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•17 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•17 years ago
|
||
Would it be difficult or impossible to reuse the textframe, but still update DOM properly?
Comment 3•17 years ago
|
||
Nominating for blocking since this is a performance spinoff of 386769
Flags: blocking1.9?
Comment 4•17 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•17 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.
Reporter | ||
Comment 7•17 years ago
|
||
> 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•17 years ago
|
||
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•17 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
Reporter | ||
Comment 10•17 years ago
|
||
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•17 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•17 years ago
|
||
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)
Reporter | ||
Comment 14•17 years ago
|
||
That code breaks with ::first-letter, I believe.
Assignee | ||
Comment 15•17 years ago
|
||
ah, probably.
Assignee | ||
Comment 16•17 years ago
|
||
But looking at nsCSSFrameConstructor::CharacterDataChanged, shouldn't be difficult to fix.
Assignee | ||
Comment 17•17 years ago
|
||
Use the normal code path when changing chardata.
Attachment #289737 -
Attachment is obsolete: true
Assignee | ||
Comment 18•17 years ago
|
||
Assignee | ||
Comment 19•17 years ago
|
||
On my machine current opt trunk (-O3) avg 82, with textframe reuse avg 68, textnode reuse avg 63.
Assignee | ||
Comment 20•17 years ago
|
||
But textframe patch doesn't quite work yet with long texts. Need to clean up something I guess.
Assignee | ||
Comment 21•17 years ago
|
||
Assignee | ||
Comment 22•17 years ago
|
||
Assignee | ||
Comment 23•17 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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
||
So - do we want to try and do this for 1.9 or not?
Comment 37•17 years ago
|
||
No motion - moving off blocking list for now. Re-nom if you disagree
Flags: blocking1.9+ → blocking1.9-
Comment 38•12 years ago
|
||
See also bug 725221, same thing for setting textContent.
Updated•11 years ago
|
Component: DOM: Mozilla Extensions → DOM
Comment 39•10 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•10 years ago
|
||
This was partially fixed in bug 922681, and bug 959150 will help in case of long text.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Assignee | ||
Comment 41•2 years ago
|
||
We have optimized innerHTML in many other ways and at least locally those testcases run quite a bit faster in Nightly than in Chrome.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•