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
Attachments
patch (10.50 KB, patch)
2012-11-01 10:50 PDT, Antti Koivisto
ojan: review+
webkit.review.bot: commit-queue-
Andreas Kling
Comment 2 2012-10-31 12:04:06 PDT
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
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.
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.