Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users
Status: Fixed
Owner:
NOT IN USE
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 567021



Sign in to add a comment
setAttribute after appending a stylesheet is linear in the number of sheets
Project Member Reported by esprehn@chromium.org, Apr 14 2016 Back to list
Google 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.
 
This is a reduced test case that came from Polymer, this bug is making web components content slow since it interleaves setAttribute and inserting sheets many times.
div-append-setAttr.html
414 bytes View Download
Cc: ericwilligers@chromium.org
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.

Comment 3 by r...@opera.com, Apr 14 2016
Blockedon: 567021
> 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 
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.
Comment 5 by r...@opera.com, 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.

Labels: Performance
Comment 8 by r...@opera.com, 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.

Comment 9 by hayato@chromium.org, Oct 12 2016
Components: -Blink>WebComponents
Comment 10 by r...@opera.com, Jan 2 2017
Status: Started
Project Member Comment 11 by bugdroid1@chromium.org, 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

Project Member Comment 12 by bugdroid1@chromium.org, Jan 4 2017
Comment 13 by r...@opera.com, Jan 6 2017
Status: Fixed
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.

Project Member Comment 14 by bugdroid1@chromium.org, 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