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

StyleInvalidationAnalysis doesn't need to always do full subtree invalidations

Reported by eseidel@chromium.org, Nov 12 2013

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.
 
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?
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
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.  
Thanks for the workaround Eric. Elliott, have you been able to make any progress?
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
Chris, what's the latest status on this?
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.
Fantastic, thanks so much.
Project Member

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

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
-----------------------------------------------------------------
Are we all set here?
No. More refactoring needed.
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.
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/
Cc: kochi@chromium.org
Chris: Should this still be open?  I was looking for the meta bug for the Style Invalidation Analysis work and found this one.
Yes, should be open, because this will be a nice cleanup and generalization of Style Invalidation.
Labels: -Cr-Blink-Rendering Cr-Blink-Layout
Migrate from Cr-Blink-Rendering to Cr-Blink-Layout
Project Member

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

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
rune@ will hopefully just delete the StyleInvalidationAnalysis class entirely. :)
Owner: r...@opera.com
Status: Assigned (was: Available)
@rune, can we block this on another issue?
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)
Project Member

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

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