[webkit-reviews] review granted: [Bug 100873] REGRESSION (r132941): attribute modification 10% performance regression : [Attachment 171893] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 1 11:30:18 PDT 2012


Ojan Vafai <ojan at chromium.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 100873: REGRESSION (r132941): attribute modification 10% performance
regression
https://bugs.webkit.org/show_bug.cgi?id=100873

Attachment 171893: patch
https://bugs.webkit.org/attachment.cgi?id=171893&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.


More information about the webkit-reviews mailing list