Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-typed-om] Prevent CSS Typed OM updates via attributeStyleMap from updating DOM style attributes #997

Closed
trusktr opened this issue Jul 25, 2020 · 4 comments

Comments

@trusktr
Copy link

trusktr commented Jul 25, 2020

Continuing from #996, personally I was hoping that modifying el.attributeStyleMap would not be reflected back to the DOM style attribute, for performance gains.

Seems half of that desire is already met in that attributeChangedCallback is not triggered.

Can we prevent dynamic modification of the CSS Typed OM from modifying the DOM style attributes?

Here's a test I made, Bunnymark DOM V1 (CSS Typed OM via attributeStyleMap), in which I was hoping to reduce performance cost by updating continually-held references to the CSS Typed OM CSSTransformValues of each bunny, then setting them with bunny.attributeStyleMap.set('transform', transform).

The more bunnies there are, the more DOM elements there are that will have updated style attributes.

I'm not sure how much of a performance improvement not updating style attribute would have.

At the moment, I see in Chrome's devtools Performance tab that most time is spent in Recalculate Style and Update Layer Tree. (I'm curious why those take so long.)

@trusktr trusktr changed the title [css-typed-om] [css-typed-om] Prevent CSS Typed OM updates via attributeStyleMap from updating DOM style attributes Jul 25, 2020
@trusktr
Copy link
Author

trusktr commented Jul 25, 2020

Alright, here's Bunnymark DOM V2 (CSS Typed OM via <style> element) which is setting individual classes for each bunny in a <style> element, but it is much slower.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Concern about attributeStyleMap updating the style attribute.

The full IRC log of that discussion <TabAtkins> Topic: Concern about attributeStyleMap updating the style attribute
<TabAtkins> github: https://github.com//issues/997
<fantasai> TabAtkins: I'm not sure if this is an actual concern to worry about
<fantasai> TabAtkins: wanted impl advice
<fantasai> TabAtkins: when doing high-frequency updates, like during TypedOM
<fantasai> TabAtkins: use single transform object, setting over and over again
<fantasai> TabAtkins: but are seeing the style attr get updated in real time as well
<fantasai> TabAtkins: so reserialization happening constantly as well
<fantasai> TabAtkins: we saw as a perf concern
<fantasai> TabAtkins: So is this actually causing trouble?
<fantasai> TabAtkins: person seems to believe causing perf issues, I'm not sure myself
<fantasai> TabAtkins: but if we are eagerly reserializing to style attr, not great. Negates a major advantage of TypedOM
<fantasai> TabAtkins: Apparently we don't ???
<fantasai> TabAtkins: but if you look in inspector, do see it being constantly updated
<fantasai> TabAtkins: But either way..
<jensimmons> present-
<fantasai> emilio: Mutation observer thing, it should fire mutation observer callbacks exceptin some cases
<fantasai> emilio: afaict, internally we don't serialize until asked for
<fantasai> emilio: if that's devtools, then we reserialize all the time
<fantasai> TabAtkins: if that's true across browsers, then I can close as no change, browsers are smart enough to handle appropriately don't worry about it
<fantasai> fremy: meaning?
<fantasai> TabAtkins: browsers aren't eagerly serializing
<fantasai> iank_: file a bug against Chrome? we should investigate this
<fantasai> TabAtkins: apparently Chrome doesn't fire mutation events
<fantasai> TabAtkins: that's a bug in chrome
<fantasai> fremy: Because we still want to update the style attr, every single time
<fantasai> fremy: can we put something in the spec that says when?
<fantasai> AmeliaBR: it's on demand
<fantasai> TabAtkins: when someone asks for it, or is observing the style attr
<fantasai> fremy: so flag it?
<fantasai> fremy: ok
<Rossen___> q?
<fantasai> TabAtkins: sounds like the resolution should be, browsers should be smart enough to lazily serialize style attr when necessary
<Zakim> fantasai, you wanted to suggest a note in the spec
<fantasai> TabAtkins: I can put a note in the spec
<fantasai> AmeliaBR: One thing to consider, should this be a general thing for DOM attributes reflected?
<fantasai> AmeliaBR: working with CSSOM or other things to be consistent?
<fantasai> TabAtkins: yeah.....
<smfr> q+
<fantasai> TabAtkins: existing string OM, not doing any restringifying, but sure there are some reflected attrs
<fantasai> TabAtkins: in HTML... I'll check if HTML has wording on that
<fantasai> ACTION: TabAtkins investigate restringification of reflected attrs across OMs
<fantasai> fremy: Not always the user that gets the style attr
<fantasai> fremy: e.g. ??
<fantasai> fremy: Should never happen. Shouldn't change the DOM when recomputing CSS
<TabAtkins> s/??/if you have a selector that depends on the style attribute's contents/
<fantasai> TabAtkins: DOM has it internally tracked, doesn't need to update everything to keep consistent except on demand
<fantasai> emilio: style attr keep not a string, but a CSS declaration and an attached string
<fantasai> emilio: when request string, we serialize it
<fantasai> emilio: Someon requests DOM attr, will get the correct value
<TabAtkins> ack smfr
<fantasai> smfr: Normally we don't specify internal details
<fantasai> smfr: Would just be a QoI issue about when serialization happens
<fantasai> smfr: Spec shouldn't say anything, should just specify web-exposed behavior
<fantasai> TabAtkins: agree
<fantasai> TabAtkins: I will close no change, review HTML to see if they have any particular wording
<fantasai> AmeliaBR: only other thing is to make sure wording is clear wrt event hooks
<fantasai> AmeliaBR: should trigger mutation observers even if not serialized until asked for
<fantasai> ACTION TabAtkins write a test for mutation observer for style attr changes via TYpedOM
<fantasai> ACTION: TabAtkins write a test for mutation observer for style attr changes via TYpedOM

@tabatkins
Copy link
Member

Closing as No Change. This is definitely meant to update the style attribute, but browsers should definitely also be optimizing and only creating the value when necessary: when you have the Inspector open, when you have an attributeChanged callback, when you have a [style="foo"] selector, etc.

So if you're observing a slowdown you think is due to stringification occurring even when it's not being observed (a little hard to tell, but I guess similar timing between "definitely observing" and "not observing" cases would probably show it), that's a browser quality-of-implementation bug.

@trusktr
Copy link
Author

trusktr commented Aug 5, 2020

I made two tests. For each test,

  • let it run without opening devtools for 10 seconds or so (to ensure no deoptimization from devtools),
  • and make sure the window size for the drawing area is the same for both in case that makes any difference.

These are the tests:

After you wait for a while (f.e. 10 seconds), open devtools for each test, and you'll see the average results outputted for every 100 animation frames.

What I found is that the average speed for the first one is almost 4 times faster. I suppose this means that in the second test the reading of the style attribute value does in fact de-optimize (as expected), so this means Chrome seems to have the above optimization in place.

I'm in Linux, so I can't test in Safari. Firefox doesn't have attributeStyleMap yet, so I didn't test there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants