Bug 100873 - REGRESSION (r132941): attribute modification 10% performance regression
: REGRESSION (r132941): attribute modification 10% performance regression
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-10-31 11:41 PST by
Modified: 2012-11-01 18:12 PST (History)


Attachments
patch (10.50 KB, patch)
2012-11-01 10:50 PST, Antti Koivisto
ojan: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #2 From 2012-10-31 12:04:06 PST -------
If it was http://trac.webkit.org/changeset/132941, I think http://trac.webkit.org/changeset/133021 should fix it.
------- Comment #3 From 2012-10-31 12:48:18 PST -------
(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.
------- Comment #4 From 2012-10-31 12:51:39 PST -------
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.
------- Comment #5 From 2012-10-31 13:10:35 PST -------
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.
------- Comment #6 From 2012-11-01 10:50:51 PST -------
Created an attachment (id=171893) [details]
patch
------- Comment #7 From 2012-11-01 11:31:40 PST -------
(From update of attachment 171893 [details])
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.
------- Comment #8 From 2012-11-01 11:45:05 PST -------
(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).
------- Comment #9 From 2012-11-01 11:54:38 PST -------
(In reply to comment #8)
> styleChangeType() < FullStyleChange is the equivalent test (more correct in fact).

Ah. Got it.
------- Comment #10 From 2012-11-01 12:07:39 PST -------
(From update of attachment 171893 [details])
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
------- Comment #11 From 2012-11-01 12:41:53 PST -------
http://trac.webkit.org/changeset/133214 (with the test breaking bug fixed)
------- Comment #12 From 2012-11-01 12:42:13 PST -------
Lets see what the bots say.
------- Comment #14 From 2012-11-01 18:12:01 PST -------
Yup. Looks great!