[Webkit-unassigned] [Bug 100873] REGRESSION (r132941): attribute modification 10% performance regression

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=100873


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #171893|review?                     |review+
               Flag|                            |




--- Comment #7 from Ojan Vafai <ojan at chromium.org>  2012-11-01 11:31:40 PST ---
(From update of attachment 171893)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.