WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r118508
: <
http://trac.webkit.org/changeset/118508
>
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.
Top of Page
Format For Printing
XML
Clone This Bug