setAttribute after appending a stylesheet is linear in the number of sheets |
|||||||
Issue descriptionGoogle Chrome 52.0.2707.0 (Official Build) canary (64-bit) Revision 52d5c52fa34e5d312e2622b7799111bb63781403-refs/heads/master@{#386876} <!DOCTYPE html> <style></style> <script> setTimeout(function() { var start = performance.now(); for (var i=0, e; i < 1000; i++) { e = document.createElement('div'); e.createShadowRoot().appendChild(document.createElement('style')); document.body.appendChild(e); e.setAttribute('foo', 'foo'); } console.log((performance.now() - start).toFixed(2)); }, 100); </script> results in 100ms, if you change the 'style' to a 'div', it takes 6ms (on my MBP). A profile shows: Running Time Symbol Name 23350.0ms 100.0% blink::DOMTimer::fired() 22835.0ms 97.7% blink::Element::setAttribute 22808.0ms 97.6% blink::Element::appendAttributeInternal 22805.0ms 97.6% blink::Element::willModifyAttribute 22801.0ms 97.6% blink::StyleEngine::attributeChangedForElement 22743.0ms 97.4% blink::StyleResolver::finishAppendAuthorStyleSheets 22722.0ms 97.3% blink::StyleResolver::collectFeatures 22205.0ms 95.0% blink::StyleEngine::collectScopedStyleFeaturesTo This is because we do: ensureResolver().ensureUpdatedRuleFeatureSet() which then walks every sheet in the entire document trying to rebuild the rule sets. I don't think we need to process pending sheets for attribute change mutations, the pending sheet should apply invalidation sets when it loads async or when a style recalc/layout is forced.
,
Apr 14 2016
The check in attributeChangedForElement does this:
if (shouldSkipInvalidationFor(element))
return;
which checks
return element.parentNode()->getStyleChangeType() >= SubtreeStyleChange;
I think the reason this checks parentNode() instead of the element itself is to handle sibling selectors? Before the sibling invalidator this used to check the element itself IIRC.
The good news there is that doing:
var wrapper = document.createElement("div");
document.body.appendChild(wrapper);
for (var i=0, e; i < 1000; i++) {
e = document.createElement('div');
e.createShadowRoot().appendChild(document.createElement('style'));
wrapper.appendChild(e);
e.setAttribute('foo', 'foo');
}
removes the n^2 in the number of sheets, and most polymer apps are not inserting thousands of siblings, they're inserting maybe 20, and those 20 contain lots of nested stuff which wouldn't cause the sheet update.
,
Apr 14 2016
> which then walks every sheet in the entire document trying to rebuild the rule sets. I don't think we need to process pending sheets for attribute change mutations, the pending sheet should apply invalidation sets when it loads async or when a style recalc/layout is forced.
Currently, invalidation sets are out-of-sync with analyzed-stylesheet-update because the former is lazily constructed while the latter happens as the stylesheet finishes parsing.
If you have this document:
<body><div id=x></div>
and do this:
document.head.innerHTML("<style>.green { color: green }</style>");
x.className = "green";
getComputedStyle(x).color;
the analyzed-stylesheet-update will not mark any elements for recalc because there are no elements with class "green" when it happens. If we didn't update the rule features when setting x.className, we would not have invalidated anything since there are no rules invalidating class="green".
This all boils down to issue 567021
,
Apr 14 2016
Right, we need to process stylesheets async, so when the sheet is finally processed in the .color accessor we'd schedule the .green invalidation set.
,
Apr 14 2016
> which checks > > return element.parentNode()->getStyleChangeType() >= SubtreeStyleChange; > > I think the reason this checks parentNode() instead of the element itself is to handle sibling selectors? Before the sibling invalidator this used to check the element itself IIRC. Yes, this changed when SubtreeStyleChange became a pure subtree change and a sibling invalidation could invalidate some sibling subtree.
,
Apr 14 2016
,
Apr 29 2016
,
Jun 28 2016
The testcase in the description now takes 5-6ms with PS18 of https://codereview.chromium.org/1913833002/ (Linux content_shell). It takes 70-80ms in my desktop Opera.
,
Oct 12 2016
,
Jan 2 2017
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26cb3bdcd2a353402b78b716862567226317dff0 commit 26cb3bdcd2a353402b78b716862567226317dff0 Author: rune <rune@opera.com> Date: Tue Jan 03 13:29:14 2017 Don't post multiple task for executing blocked scripts. We may post a lot of tasks for executing blocked scripts in the case where we insert multiple shadow trees each containing a style element. When we start parsing a style element, we mark it as blocking and unblock script execution after finishing parsing. Check if the previous task is active before posting. Found while measuring performance for issue 603621 . BUG= 603621 Review-Url: https://codereview.chromium.org/2609763002 Cr-Commit-Position: refs/heads/master@{#441110} [modify] https://crrev.com/26cb3bdcd2a353402b78b716862567226317dff0/third_party/WebKit/Source/core/dom/Document.cpp
,
Jan 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0 commit f831fc79824c0281cdd4f6a90d7bc92dbadd44b0 Author: rune <rune@opera.com> Date: Wed Jan 04 11:54:50 2017 Avoid unnecessary updateActiveStyle comparing shadow styles. Found while checking performance for issue 603621 . Style sharing is done during style recalc at which point we know that the active style is up-to-date. Instead of using the API for document.styleSheets, compare active stylesheets in ScopedStyleResolver directly. BUG= 603621 Review-Url: https://codereview.chromium.org/2610513003 Cr-Commit-Position: refs/heads/master@{#441357} [modify] https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0/third_party/WebKit/Source/core/BUILD.gn [modify] https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp [modify] https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h [add] https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolverTest.cpp [modify] https://crrev.com/f831fc79824c0281cdd4f6a90d7bc92dbadd44b0/third_party/WebKit/Source/core/dom/shadow/ElementShadow.cpp
,
Jan 6 2017
According to callgrind, setAttribute is now only spending time in the DOM parts of the code. The numbers I see in the console log is higher than the ones I mentioned in June, both with and without the fix. Changing the element count from 1000 to 10000 really shows the improvement as my ToT content_shell uses less than 200ms while Chrome version 57.0.2950.4 uses almost 14s. The two fixes I've landed (one of which is about to be reverted) were merely minor fixes of things identified running it through callgrind. I'll mark this as fixed now.
,
Jan 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/482c1f85b922eeec0b9525838baca4b884c27392 commit 482c1f85b922eeec0b9525838baca4b884c27392 Author: rune <rune@opera.com> Date: Fri Jan 06 01:20:43 2017 Revert of Don't post multiple task for executing blocked scripts. (patchset #2 id:20001 of https://codereview.chromium.org/2609763002/ ) Reason for revert: Regression in test for time to first meaningful paint: https://crbug.com/678584 Original issue's description: > Don't post multiple task for executing blocked scripts. > > We may post a lot of tasks for executing blocked scripts in the case > where we insert multiple shadow trees each containing a style element. > When we start parsing a style element, we mark it as blocking and > unblock script execution after finishing parsing. Check if the previous > task is active before posting. > > Found while measuring performance for issue 603621 . > > BUG= 603621 > > Committed: https://crrev.com/26cb3bdcd2a353402b78b716862567226317dff0 > Cr-Commit-Position: refs/heads/master@{#441110} TBR=pmeenan@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 603621 Review-Url: https://codereview.chromium.org/2617763002 Cr-Commit-Position: refs/heads/master@{#441783} [modify] https://crrev.com/482c1f85b922eeec0b9525838baca4b884c27392/third_party/WebKit/Source/core/dom/Document.cpp |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by esprehn@chromium.org
, Apr 14 2016414 bytes
414 bytes View Download