Bug 100873 – REGRESSION (r132941): attribute modification 10% performance regression

• Component: DOM
• Status: RESOLVED FIXED

[Ojan Vafai]

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

[Ojan Vafai]

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]

If it was http://trac.webkit.org/changeset/132941, I think http://trac.webkit.org/changeset/133021 should fix it.

[Ojan Vafai]

(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]

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]

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]

Created attachment 171893 [details]
patch

[Ojan Vafai]

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]

(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]

(In reply to comment #8)
> styleChangeType() < FullStyleChange is the equivalent test (more correct in fact).

Ah. Got it.

[WebKit Review Bot]

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]

http://trac.webkit.org/changeset/133214 (with the test breaking bug fixed)

[Antti Koivisto]

Lets see what the bots say.

[Antti Koivisto]

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]

Yup. Looks great!