Bug 87211 - Dodge style recalc when id attribute is overwritten with same value.
Summary: Dodge style recalc when id attribute is overwritten with same value.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-22 22:09 PDT by Andreas Kling
Modified: 2012-06-01 07:47 PDT (History)
5 users (show)

See Also:


Attachments
Quick hack (1.75 KB, patch)
2012-05-22 22:11 PDT, Andreas Kling
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2012-05-22 22:09:51 PDT
Looks like this could help on Dromaeo/dom-attr.
Comment 1 Andreas Kling 2012-05-22 22:11:49 PDT
Created attachment 143461 [details]
Quick hack
Comment 2 Kenneth Rohde Christiansen 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?
Comment 3 Caio Marcelo de Oliveira Filho 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?
Comment 4 Caio Marcelo de Oliveira Filho 2012-05-23 06:16:59 PDT
Idea: could we be smarter in setAttributeInternal about changing attributes to the same value, and bailing earlier?
Comment 5 Andreas Kling 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.
Comment 6 Eric Seidel (no email) 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.
Comment 7 Andreas Kling 2012-05-25 03:09:51 PDT
Committed r118508: <http://trac.webkit.org/changeset/118508>
Comment 8 Ojan Vafai 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.
Comment 9 Andreas Kling 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. :)
Comment 10 Ojan Vafai 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.
Comment 11 Antti Koivisto 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.