Looks like this could help on Dromaeo/dom-attr.
Created attachment 143461 [details] Quick hack
(In reply to comment #1) > Created an attachment (id=143461) [details] > Quick hack Why is that a hack? How else would you do it?
Who's calling attributeChanged() in this scenario? Couldn't we avoid calling attributeChanged() when the new value is equals to the old value?
Idea: could we be smarter in setAttributeInternal about changing attributes to the same value, and bailing earlier?
(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.
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.
Committed r118508: <http://trac.webkit.org/changeset/118508>
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.
(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. :)
(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.
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.