Bug 13602 – Amazon product pages keep repainting over and over again

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

If you go to an Amazon product page and turn on Quartz Debug, you'll see the entire page painting over and over again.

This is bad for your computer's battery!

These pages have a timer that keeps calling setInnerHTML over and over again. The solution to this is to make setInnerHTML more sophisticated about not changing the DOM if the new HTML is the same as what's already there.

Why is the entire page being repainted? Is it setting the HTML for most of the page?

[Ian 'Hixie' Hickson]

innerHTML should always blow away the DOM, though, even if the new value equals the old value. No?

[Darin Adler]

(In reply to comment #2)
> innerHTML should always blow away the DOM, though, even if the new value equals
> the old value. No?

Why? Is that absolutely necessary?

[Darin Adler]

(In reply to comment #1)
> Why is the entire page being repainted? Is it setting the HTML for most of the
> page?

It's not really the entire page. More of the page has to be repainted than you might expect because of the reflow algorithm and floats. Hyatt says that with some work we can make it repaint less. Gecko repaints less.

[Darin Adler]

Created attachment 14378 [details]
patch

[Dave Hyatt]

Comment on attachment 14378 [details]
patch

Awesome patch, needs layout test though.  Should be able to test with an incremental repaint test right?

[Dave Hyatt]

Floats cause us to repaint too much.  Line layout is still totally unoptimized for repainting when floats are present. :(

[Darin Adler]

(In reply to comment #6)
> Awesome patch, needs layout test though.  Should be able to test with an
> incremental repaint test right?

I don't know how to write a repaint test.

But I was thinking of instead writing a test to verify that the DOM is not molested.

[Darin Adler]

Created attachment 14382 [details]
patch with test

[Dave Hyatt]

Comment on attachment 14382 [details]
patch with test

r=me

[Ian 'Hixie' Hickson]

> > innerHTML should always blow away the DOM, though, even if the new value 
> > equals the old value. No?
> 
> Why? Is that absolutely necessary?

It would cause all kinds of weird bugs if we didn't, since it would mean that a DOM script that got a handle to one of the nodes in the tree would find that node was in the Document even when that isn't expected.

[Darin Adler]

(In reply to comment #11)
> > > innerHTML should always blow away the DOM, though, even if the new value 
> > > equals the old value. No?
> > 
> > Why? Is that absolutely necessary?
> 
> It would cause all kinds of weird bugs if we didn't, since it would mean that a
> DOM script that got a handle to one of the nodes in the tree would find that
> node was in the Document even when that isn't expected.

In theory it might be possible to make that assumption. In practice it seems highly unlikely.

I think that if you change the value of the innerHTML property to a new value and that matches the old value, it's not reasonable to rely on the fact that setting the property's value is guaranteed to obliterate the old DOM nodes. And it's not typical to rely on it one way or another.

Pages that redundantly set innerHTML or innerText without modifying anything are pretty common, so there's a clear real-world benefit to not mutating the DOM at all, and only a theoretical benefit to guaranteeing all the nodes will be obliterated any time you set the value of the innerHTML/innerText property.

So I think I still like this change.

What will it take to convince you?

[Dave Hyatt]

I have to come down on the side of practicality here.  Do we really want all Amazon product pages repainting every 20ms because we didn't have this optimization?  I don't think so. 

Put the optimization in the spec, and then it won't be ambiguous what happens. :)

My r=me stands.

[Ian 'Hixie' Hickson]

> What will it take to convince you?

Oh I'm convinced that this would be a good optimisation. I'm sure, however, that there are pages that rely on the way IE does this. I guess we'll find out. (The patch looks more complicated than just comparing innerHTML to itself before setting it, though?)

[Darin Adler]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/dom/HTMLElement/set-inner-outer-optimization-expected.txt
Adding         LayoutTests/fast/dom/HTMLElement/set-inner-outer-optimization.html
Sending        WebCore/ChangeLog
Sending        WebCore/dom/CharacterData.cpp
Sending        WebCore/html/HTMLElement.cpp
Transmitting file data ......
Committed revision 21284.

[Darin Adler]

Other layout tests were failing -- I rolled it out until I can figure out why.

[Darin Adler]

Created attachment 14398 [details]
new patch, now with passing layout tests (oops!)

[Darin Adler]

Comment on attachment 14382 [details]
patch with test

Clearing review flag from this because it broke layout tests.

[Maciej Stachowiak]

Comment on attachment 14398 [details]
new patch, now with passing layout tests (oops!)


In +static bool shallowEqual(Node* a, Node* b) you have a comment:

+            // Don't bother comparing these node types for now; they don't typically occur.
+            // Since these functions are used only for optimization, it's OK to have a false negative.

Some of these node types literally can't ever appear inside a document tree (at least document, document fragment, and xpath namespace nodes), not sure if it is worth putting a different comment for those.

replaceChildrenWithFragment() codes the algorithm to compare whether children are identical. Would it not be better to factor that out into a contentsAreEqual() function?

The optimizations for replacing a single child and for replacing a sole text child with different text are a bit hard to follow at first. (a) are these really helpful in real cases? (b) could the if statements be coded to use functions that have clear names for what they are checking? Perhaps similar for replaceChildrenWithText.

setOuterHTML also has a deep identical contents check, perhaps refactoring would allow this to be shared with setInnerHTML more.

r=me notwithstanding the style comments.

[Darin Adler]

(In reply to comment #19)
> Some of these node types literally can't ever appear inside a document tree (at
> least document, document fragment, and xpath namespace nodes), not sure if it
> is worth putting a different comment for those.

I changed the existing comment. The function itself could be used outside a document tree, so I figure it's not all that different a case.

> replaceChildrenWithFragment() codes the algorithm to compare whether children
> are identical. Would it not be better to factor that out into a
> contentsAreEqual() function?

Done.

> The optimizations for replacing a single child and for replacing a sole text
> child with different text are a bit hard to follow at first. (a) are these
> really helpful in real cases?

I'm not sure. I suspect the "replace text with other text" case is common.

> (b) could the if statements be coded to use
> functions that have clear names for what they are checking? Perhaps similar for
> replaceChildrenWithText.

Done.

> setOuterHTML also has a deep identical contents check, perhaps refactoring
> would allow this to be shared with setInnerHTML more.

Done.

[Darin Adler]

Committed revision 21861.