Bug 87211 – Dodge style recalc when id attribute is overwritten with same value.

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

[Andreas Kling]

Looks like this could help on Dromaeo/dom-attr.

[Andreas Kling]

Created attachment 143461 [details]
Quick hack

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> Created an attachment (id=143461) [details]
> Quick hack

Why is that a hack? How else would you do it?

[Caio Marcelo de Oliveira Filho]

Who's calling attributeChanged() in this scenario? Couldn't we avoid calling attributeChanged() when the new value is equals to the old value?

[Caio Marcelo de Oliveira Filho]

Idea: could we be smarter in setAttributeInternal about changing attributes to the same value, and bailing earlier?

[Andreas Kling]

(In reply to comment #3)
> Who's calling attributeChanged() in this scenario? Couldn't we avoid calling attributeChanged() when the new value is equals to the old value?

I've been down this road before, and it's a bit rocky. See bug 80441.

[Eric Seidel (no email)]

Comment on attachment 143461 [details]
Quick hack

I would have written this differently.  Probably added a static inline function to compute the new effective ID, and then done the compare, set and recalc all in the same if.   But unifying the set's as such could be viewed as a separate cleanup.

[Andreas Kling]

Committed r118508: <http://trac.webkit.org/changeset/118508>

[Ojan Vafai]

Do we have any data that real-world sites do this? It seems unlikely to me that there is much real-world content that sets an id to the same value.

This feels a little too much like gaming the benchmark instead of actually fixing real problems (admittedly, the benchmark is doing something stupid). I'd prefer if we gathered real data before making changes like this.

[Andreas Kling]

(In reply to comment #8)
> Do we have any data that real-world sites do this? It seems unlikely to me that there is much real-world content that sets an id to the same value.
> 
> This feels a little too much like gaming the benchmark instead of actually fixing real problems (admittedly, the benchmark is doing something stupid). I'd prefer if we gathered real data before making changes like this.

While I agree that this kind of change could be considered a benchmark hack, I think of this particular one as fixing a silly inefficiency (potentially expensive style recalc triggered by a no-op) that was highlighted by a benchmark that's supposed to test something else. If it weren't a trivial one-liner, I would probably feel differently. :)

[Ojan Vafai]

(In reply to comment #9)
> (In reply to comment #8)
> > Do we have any data that real-world sites do this? It seems unlikely to me that there is much real-world content that sets an id to the same value.
> > 
> > This feels a little too much like gaming the benchmark instead of actually fixing real problems (admittedly, the benchmark is doing something stupid). I'd prefer if we gathered real data before making changes like this.
> 
> While I agree that this kind of change could be considered a benchmark hack, I think of this particular one as fixing a silly inefficiency (potentially expensive style recalc triggered by a no-op) that was highlighted by a benchmark that's supposed to test something else. If it weren't a trivial one-liner, I would probably feel differently. :)

Fair enough. I suppose it's a reasonable optimization to do given the benefit, even if it's a silly thing that will happen rarely in the real-world. It still smells wrong to me. :)

Would be nice to add a perf test that does what dom-attr does, but sets different values on each iteration so that we can have a perf test that goes through the recalc codepath as well.

FWIW, on the chromium perf bots we saw a 7% improvement from this patch.

[Antti Koivisto]

I think we should generally optimize for setting things to the existing value. It is an easy mistake to make and cheap to optimize for.