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 8 users

Issue metadata

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
link

Issue 318468: StyleInvalidationAnalysis doesn't need to always do full subtree invalidations

Reported by eseidel@chromium.org, Nov 12 2013 Project Member

Issue description

StyleInvalidationAnalysis doesn't need to always do full subtree invalidations

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/css/StyleInvalidationAnalysis.cpp&q=invalidateStyle&sq=package:chromium&l=207

Right now we're smart enough to only invalidate classes/ids which are mentioned in the new/removed stylesheet, but we're not smart enough to only invalidate the elements changed, we always invalidate their whole subtrees.

Imagine for example adding the following sheet:
<style>body { background-color: blue; }</style>
we'd needlessly re-style the whole document.
 

Comment 1 by eseidel@chromium.org, Nov 12 2013

Cc: baker@google.com
This comes up because Baker's app needs to change the overflow rule for a large subtree and does so by adding overflow: hidden; on a root element.  Currently they do that as part of adding their "animating" stylesheet, which seems entirely reasonable.

I think they could work around this by manually doing:
root.style.overflow = "hidden" at the start of their animation, but it seems nicer to do it via a stylesheet, we just need to be smarter.

Comment 2 by nduca@chromium.org, Nov 12 2013

An (albeit contrived) case: http://codepen.io/anon/pen/pfqnJ

Comment 3 by nduca@chromium.org, Nov 13 2013

Labels: SlowLayoutOrRecalcStyle

Comment 4 by nduca@chromium.org, Nov 14 2013

Cc: ybo@google.com
Labels: Hotlist-GoogleApps
Needed for Music. Elliot, can you own?

Comment 5 by esprehn@chromium.org, Nov 14 2013

Sure, looking at determineSelectorScopes() we intentionally inflate the invalidations scopes to avoid having too many scopes. We need to figure out when it's safe not to inflate the scope and when LocalStyleChange is okay.

Comment 6 Deleted

Comment 7 by nduca@chromium.org, Nov 15 2013

Owner: esprehn@chromium.org
Status: Assigned

Comment 8 by eseidel@chromium.org, Nov 23 2013

You can work around this bug by applying your CSS via javascript.

For example if you wanted to add overflow: hidden; to the <body> element and not have it recalc all descendants currently:

var style = document.createElement("style");
style.innerText = '#body { overflow: hidden }';
document.body.appendChild(style);

Will hit the "fast" StyleInvalidationAnalysis path, but will recalc too much due to this bug.

but document.body.style.overflow = 'hidden'; should only cause the <body> to recalc w/o recalculating children.  I think.

Comment 9 by baker@google.com, Dec 9 2013

Just confirming Eric's work around does avoid recalculating the whole sub-tree.

Comment 10 by rsch...@chromium.org, Jan 6 2014

Thanks for the workaround Eric. Elliott, have you been able to make any progress?

Comment 11 by rsch...@chromium.org, Jan 28 2014

Ping on this. Any updates?

Comment 12 by r...@opera.com, Feb 16 2014

Cc: r...@opera.com

Comment 13 by baker@google.com, Feb 18 2014

Owner: chrishtr@chromium.org

Comment 14 by rsch...@chromium.org, Mar 24 2014

Chris, what's the latest status on this?

Comment 15 by chrishtr@chromium.org, Mar 24 2014

I was just looking at it today actually. It should be simple given my refactoring in the last week to fix this, am planning to send out a CL soon.

Comment 16 by rsch...@chromium.org, Mar 24 2014

Fantastic, thanks so much.

Comment 18 by bugdroid1@chromium.org, Apr 3 2014

Project Member
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=170719

------------------------------------------------------------------
r170719 | chrishtr@chromium.org | 2014-04-03T00:17:25.049018Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/invalidation/DescendantInvalidationSet.h?r1=170719&r2=170718&pathrev=170719
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/invalidation/targeted-id-style-invalidation.html?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/invalidation/StyleInvalidator.cpp?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/Element.cpp?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/RuleFeature.cpp?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/invalidation/StyleInvalidator.h?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/invalidation/DescendantInvalidationSet.cpp?r1=170719&r2=170718&pathrev=170719
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/css/invalidation/targeted-id-style-invalidation-expected.txt?r1=170719&r2=170718&pathrev=170719
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/css/RuleFeature.h?r1=170719&r2=170718&pathrev=170719

Add support for element ids in TargetedStyleRecalc.

BUG= 318468 

Review URL: https://codereview.chromium.org/220123004
-----------------------------------------------------------------

Comment 19 by rsch...@chromium.org, Apr 29 2014

Are we all set here?

Comment 20 by chrishtr@chromium.org, Apr 29 2014

No. More refactoring needed.

Comment 21 by rsch...@chromium.org, Jun 3 2014

Sweeping the apps hotlist - any updates?

Comment 22 by r...@opera.com, Jun 4 2014

Cc: -r...@opera.com chrishtr@chromium.org
Owner: r...@opera.com
Status: Started
I volunteered to look at this a while ago. I can start now.

Comment 23 by r...@opera.com, Jun 4 2014

To my understanding, this issue is now about using descendant invalidation sets when adding stylesheets like described in https://code.google.com/p/chromium/issues/detail?id=318468#c8

Comment 24 by ojan@chromium.org, Jun 7 2014

Yup. That matches my understanding.

Comment 25 by rsch...@chromium.org, Jul 28 2014

rune, have you been able to make any progress?

Comment 26 by r...@opera.com, Aug 5 2014

Owner: ----
Status: Available
@rschoen No, I always get distracted by something else. I'll make this Available.

I'll add esprehn's resolver refactoring proposal as that is highly relevant here:

https://docs.google.com/a/opera.com/document/d/1Z13sUZoVP6swGMp2VUIyhOe9-STW3gp6-dAIVIxRRNk/

Comment 27 by kochi@chromium.org, Aug 5 2014

Cc: kochi@chromium.org

Comment 28 by eseidel@chromium.org, Sep 15 2014

Chris: Should this still be open?  I was looking for the meta bug for the Style Invalidation Analysis work and found this one.

Comment 29 by chrishtr@chromium.org, Sep 15 2014

Yes, should be open, because this will be a nice cleanup and generalization of Style Invalidation.

Comment 30 by laforge@google.com, Jan 9 2015

Labels: -Cr-Blink-Rendering Cr-Blink-Layout
Migrate from Cr-Blink-Rendering to Cr-Blink-Layout

Comment 31 by sheriffbot@chromium.org, Mar 26 2016

Project Member
Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been available for more than 365 days, and should be re-evaluated. Hotlist-Recharge-Cold label is added for tracking. Please re-triage this issue.

For more details visit https://sites.google.com/a/chromium.org/dev/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 32 by kochi@chromium.org, Mar 28 2016

Components: -Blink>Layout Blink>CSS
Moving the component to CSS so this will be triaged.

Comment 33 by loyso@chromium.org, Mar 28 2016

Labels: -OS-Chrome Performance OS-All
Status: Available (was: Untriaged)

Comment 34 by loyso@chromium.org, Mar 28 2016

Labels: -Type-Bug Type-Feature

Comment 35 by esprehn@chromium.org, Mar 28 2016

rune@ will hopefully just delete the StyleInvalidationAnalysis class entirely. :)

Comment 36 by dstockwell@chromium.org, Apr 29 2016

Owner: r...@opera.com
Status: Assigned (was: Available)
@rune, can we block this on another issue?

Comment 37 by dstockwell@chromium.org, Apr 29 2016

Labels: -Type-Feature Type-Bug

Comment 38 by r...@opera.com, Apr 29 2016

Blockedon: 567021

Comment 39 by r...@opera.com, Jan 5 2017

Status: Fixed (was: Assigned)

Comment 40 by bugdroid1@chromium.org, Jan 6 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e8e7e02ebf0123bead605c0f974badf6c8260d1e

commit e8e7e02ebf0123bead605c0f974badf6c8260d1e
Author: rune <rune@opera.com>
Date: Fri Jan 06 02:15:17 2017

Added layout test for  issue 318468 .

Check that adding a type selector rule only invalidates elements with
that type.

R=sashab@chromium.org
BUG= 318468 

Review-Url: https://codereview.chromium.org/2615713005
Cr-Commit-Position: refs/heads/master@{#441823}

[add] https://crrev.com/e8e7e02ebf0123bead605c0f974badf6c8260d1e/third_party/WebKit/LayoutTests/fast/css/invalidation/sheet-ruleset-invalidation.html

Sign in to add a comment