New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 712080 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 699838



Sign in to add a comment

Make StyleElement::processStyleSheet() in a shadow tree faster

Project Member Reported by hayato@chromium.org, Apr 17 2017

Issue description

The context: https://crbug.com/699838

Given the trace [1] and the metrics which I have written locally, a StyleElement::processStyleSheet takes about 40% (= 60/149) in <x-thing>'s connected callbacks.

The followings are total, not avg:

- CE::connected_x-thing: 
  - app1:  76.9ms                
  - app2: 149.6ms

- StyleElement::ProcessStyleSheet:
  - app1:  0.029ms                
  - app2: 60.583ms
  
- StyleElement::ProcessStyleSheet_under_x-thing:
  - app1:  (empty)
  - app2: 60.556ms
  

Some observations:
- Blink does not parse the text contents of <style> element if it hits the cache.  This behavior is working as expected.
- However, even if we don't parse the text contents in processing each style element, the cost of processing stylesheet is still high enough.
- StyleSheetContents::checkLoaded() is called every time in a processing stylesheet, and it takes 35.376 ms, which is more that 50% in StyleElement::ProcessStyleSheet()

[1]: https://bugs.chromium.org/p/chromium/issues/detail?id=699838#c8

 
processstylesheet.png
36.2 KB View Download

Comment 1 by hayato@chromium.org, Apr 17 2017

Components: Blink>CSS Blink>Loader
Let me add Blink>Loader and Blink>CSS.

As of now, I am not sure for what StyleSheetContents::checkLoaded() [1] is being used.
Let me investigate this further, but if someone has an insight, that would be welcomed.

It looks that StyleSheetContents::checkLoaded() is called every time in processing style sheet in a shadow tree. That means we would call checkLoaded more than 1K times in Shadow DOM case of bug 699838.

I am wondering whether this is really necessary or not.


[1] StyleSheetContents::checkLoaded():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/StyleSheetContents.cpp?q=checkLoaded+package:%5Echromium$&l=417

Comment 2 by hayato@chromium.org, Apr 17 2017

Status: Started (was: Assigned)

Comment 3 by hayato@chromium.org, Apr 17 2017

Hmm. Regarding checkLoaded, I have found that <style> element should fire a load event [1], and Blink actually fires a load event for each style element. That's sad. :(  I am wondering who needs this event...


[1] https://html.spec.whatwg.org/multipage/semantics.html#the-style-element

> Once the attempts to obtain the style sheet's critical subresources, if any, are complete, or, if the style sheet has no critical subresources, once the style sheet has been parsed and processed, the user agent must, if the loads were successful or there were none, queue a task to fire an event named load at the style element, or, if one of the style sheet's critical subresources failed to completely load for any reason (e.g. DNS error, HTTP 404 response, a connection being prematurely closed, unsupported Content-Type), queue a task to fire an event named error at the style element. Non-network errors in processing the style sheet or its subresources (e.g. CSS parse errors, PNG decoding errors) are not failures for the purposes of this paragraph.



Comment 4 by hayato@chromium.org, Apr 17 2017

I would say that some behaviors of <style> element are not good in Web Components era, where we have >1K custom element instances, and each shadow tree might have a <style> element inside of it. We do not want to queue a task to fire a load event for each. That looks too wasteful.

I am thinking that we should make Constructable Stylesheet Objects [1] happen seriously.

[1] http://tabatkins.github.io/specs/construct-stylesheets/
Can I have a copy of your benchmark? I can work on the connected cost.
Cc: dominicc@chromium.org

Comment 7 by hayato@chromium.org, Apr 18 2017

I have uploaded the current metrics I am using here: https://codereview.chromium.org/2760283002
Labels: Performance

Comment 9 by hayato@chromium.org, Apr 18 2017

Components: -Blink>CSS -Blink>Loader
Regarding a load event, I have filed a bug 713562.
Labels: -Performance Performance-Loading

Comment 12 by kochi@chromium.org, Jan 31 2018

Cc: kochi@chromium.org
Cc: -dominicc@chromium.org hayato@chromium.org
Owner: ----
Status: Available (was: Started)
Releasing this bug.
Maybe we might be in favor of Constructable Stylesheets.

Sign in to add a comment