RESOLVED FIXED 87211
Dodge style recalc when id attribute is overwritten with same value.
https://bugs.webkit.org/show_bug.cgi?id=87211
Summary Dodge style recalc when id attribute is overwritten with same value.
Andreas Kling
Reported 2012-05-22 22:09:51 PDT
Looks like this could help on Dromaeo/dom-attr.
Attachments
Quick hack (1.75 KB, patch)
2012-05-22 22:11 PDT, Andreas Kling
eric: review+
Andreas Kling
Comment 1 2012-05-22 22:11:49 PDT
Created attachment 143461 [details] Quick hack
Kenneth Rohde Christiansen
Comment 2 2012-05-23 00:55:07 PDT
(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
Comment 3 2012-05-23 06:13:32 PDT
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
Comment 4 2012-05-23 06:16:59 PDT
Idea: could we be smarter in setAttributeInternal about changing attributes to the same value, and bailing earlier?
Andreas Kling
Comment 5 2012-05-23 06:20:27 PDT
(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 6 2012-05-24 16:41:04 PDT
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
Comment 7 2012-05-25 03:09:51 PDT
Ojan Vafai
Comment 8 2012-05-31 13:42:59 PDT
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
Comment 9 2012-05-31 13:49:40 PDT
(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
Comment 10 2012-05-31 13:58:11 PDT
(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
Comment 11 2012-06-01 07:47:22 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.