WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
100873
REGRESSION (
r132941
): attribute modification 10% performance regression
https://bugs.webkit.org/show_bug.cgi?id=100873
Summary
REGRESSION (r132941): attribute modification 10% performance regression
Ojan Vafai
Reported
2012-10-31 11:41:02 PDT
Regression range:
http://trac.webkit.org/log/?verbose=on&rev=132949&stop_rev=132924
http://trac.webkit.org/changeset/132941/
seems like the most likely culprit to me. Most clear graph. Click on the part in the graph where the performance drops then click on the webkit link on the right to see the webkit blamelist:
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibattrjquery/report.html?rev=165231&graph=jslib_attr_jquery_jQuery___removeClass&trace=score&history=150
Some less clear graphs that point at the same webkit blamelist:
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibattrprototype/report.html?rev=165213&graph=jslib_attr_prototype_Prototype___removeClassName&trace=score&history=150
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibattrprototype/report.html?rev=165213&graph=jslib_attr_prototype_Prototype___writeAttribute&trace=score&history=150
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibattrjquery/report.html?rev=165213&graph=jslib_attr_jquery_jQuery___attr_class_test_&trace=score&history=150
Attachments
patch
(10.50 KB, patch)
2012-11-01 10:50 PDT
,
Antti Koivisto
ojan
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Ojan Vafai
Comment 1
2012-10-31 11:48:45 PDT
Smaller regression range from the Chromium linux bot:
http://trac.webkit.org/log/?verbose=on&rev=132944&stop_rev=132938
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibattrjquery/report.html?rev=165139&graph=jslib_attr_jquery_jQuery___attr_class_test_&trace=score&history=150
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibattrprototype/report.html?rev=165139&graph=jslib_attr_prototype_Prototype___removeClassName&trace=score&history=150
Andreas Kling
Comment 2
2012-10-31 12:04:06 PDT
If it was
http://trac.webkit.org/changeset/132941
, I think
http://trac.webkit.org/changeset/133021
should fix it.
Ojan Vafai
Comment 3
2012-10-31 12:48:18 PDT
(In reply to
comment #2
)
> If it was
http://trac.webkit.org/changeset/132941
, I think
http://trac.webkit.org/changeset/133021
should fix it.
133021 did not fix it.
Antti Koivisto
Comment 4
2012-10-31 12:51:39 PDT
Not sure it will. We'll see. I think a sufficiently artificial test case might see a regression from that change. There are ways to improve if needed.
Antti Koivisto
Comment 5
2012-10-31 13:10:35 PDT
Turns out the difference finding algorithm using SpaceSplitString mutation is too slow: Running Time Self Symbol Name 1714.0ms 5.6% 302,0 WebCore::Element::classAttributeChanged(WTF::AtomicString const&) 532.0ms 1.7% 174,0 WebCore::SpaceSplitString::remove(WTF::AtomicString const&) 277.0ms 0.9% 66,0 WebCore::SpaceSplitString::ensureUnique() 136.0ms 0.4% 136,0 WTF::fastMalloc(unsigned long) 75.0ms 0.2% 75,0 WTF::Vector<WTF::AtomicString, 4ul>::Vector(WTF::Vector<WTF::AtomicString, 4ul> const&) Should be easy to optimize this away.
Antti Koivisto
Comment 6
2012-11-01 10:50:51 PDT
Created
attachment 171893
[details]
patch
Ojan Vafai
Comment 7
2012-11-01 11:31:40 PDT
Comment on
attachment 171893
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=171893&action=review
> Source/WebCore/dom/Element.cpp:723 > + StyleResolver* styleResolver = document()->styleResolverIfExists(); > + bool testShouldInvalidateStyle = attached() && styleResolver && styleChangeType() < FullStyleChange;
I almost wonder if it's worth adding a static helper function here since this code is duplicated with Element::classAttributeChanged. static StyleResolver* styleResolverForTestingStyleInvalidation(Document* document, Element* element) { if (!element->attached() || element->needsStyleRecalc() || styleTypeChange >= FullStyleChange) return 0; return document()->styleResolverIfExists(); } Then you wouldn't need the testShouldInvalidateStyle at all and could just null check the returned style resolver. It's not clearly better to me, but it does avoid some duplication. Up to you.
> Source/WebCore/dom/Element.cpp:733 > + if (shouldInvalidateStyle) > + testShouldInvalidateStyle = false;
I think you could avoid needing to do this if you put the check below into an else clause. That'd simplify the logic some I think. } else { if (name == HTMLNames::nameAttr) setHasName(!newValue.isNull()); shouldInvalidateStyle = ...; }
> Source/WebCore/dom/Element.cpp:738 > + shouldInvalidateStyle = testShouldInvalidateStyle && styleResolver->hasSelectorForAttribute(name.localName());
Not sure it matters, but we currently only do this if (!needsStyleRecalc()). Maybe testShouldInvalidateStyle should also && !needsStyleRecalc()? Obviously the same would apply in Element::classAttributeChanged. Saves a hashtable lookup. Not sure it matters in practice.
Antti Koivisto
Comment 8
2012-11-01 11:45:05 PDT
(In reply to
comment #7
)
> Not sure it matters, but we currently only do this if (!needsStyleRecalc()). Maybe testShouldInvalidateStyle should also && !needsStyleRecalc()? Obviously the same would apply in Element::classAttributeChanged. Saves a hashtable lookup. Not sure it matters in practice.
styleChangeType() < FullStyleChange is the equivalent test (more correct in fact).
Ojan Vafai
Comment 9
2012-11-01 11:54:38 PDT
(In reply to
comment #8
)
> styleChangeType() < FullStyleChange is the equivalent test (more correct in fact).
Ah. Got it.
WebKit Review Bot
Comment 10
2012-11-01 12:07:39 PDT
Comment on
attachment 171893
[details]
patch
Attachment 171893
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://queues.webkit.org/results/14686598
New failing tests: css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html fast/dynamic/004.html
Antti Koivisto
Comment 11
2012-11-01 12:41:53 PDT
http://trac.webkit.org/changeset/133214
(with the test breaking bug fixed)
Antti Koivisto
Comment 12
2012-11-01 12:42:13 PDT
Lets see what the bots say.
Antti Koivisto
Comment 13
2012-11-01 16:31:21 PDT
Looks like it fixed it.
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_jslibattrjquery/report.html?graph=jslib_attr_jquery_jQuery___removeClass&trace=score&history=100&rev=165526
Ojan Vafai
Comment 14
2012-11-01 18:12:01 PDT
Yup. Looks great!
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