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)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bzbarsky, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(5 files, 1 obsolete file)

for testing, reuse the domnode
3.23 KB, patch
Details | Diff | Splinter Review
reuse textframes, not so ugly.
13.03 KB, patch
Details | Diff | Splinter Review
testcase
872 bytes, text/html
Details
testcase2, long text, :first-letter and :first-line
1.68 KB, text/html
Details
works with long text
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.
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.
Would it be difficult or impossible to reuse the textframe, but still update
DOM properly?
Nominating for blocking since this is a performance spinoff of 386769
Flags: blocking1.9?
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?
(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
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.
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.
(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.

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.
ah, probably.
But looking at nsCSSFrameConstructor::CharacterDataChanged, shouldn't
be difficult to fix.
Use the normal code path when changing chardata.
Attachment #289737 - Attachment is obsolete: true
Attached file testcase — 

    
    

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

    
    

    
  
But textframe patch doesn't quite work yet with long texts. Need to clean up something I guess.

    
    

    
  
Opt Linux 32bit build:
testcase:  avg ~90  with patch, 103 without patch
testcase2: avg ~128 with patch, 144 without patch

    
    

    
  
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.

    
    

    
  
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...

    
    

    
  
...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.

    
    

    
  
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.

    
    

    
  
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)


    
    

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

    
    

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

    
  
No longer blocks: 386769

    
  
Blocks: 442348

    
    

    
  
See also bug 725221, same thing for setting textContent.
Component: DOM: Mozilla Extensions → DOM

    
    

    
  
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

    
    

    
  
This was partially fixed in bug 922681, and bug 959150 will help in case of long text.
Component: DOM → DOM: Core & HTML

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: