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

Performance issue in CSSStyleDeclaration::set_property #17399

Open
glennw opened this Issue Jun 19, 2017 · 16 comments

Comments

Projects
None yet
7 participants
@glennw
Copy link
Member

glennw commented Jun 19, 2017

I'm profiling the Multiply test in the MotionMark benchmark suite - the major bottleneck in this test is the rAF call.

In this test, most of the time is spent in the (many) calls to set_backgroundColor and set_Transform.

s1

If we zoom in on these calls, we seem to spend a lot of time in the ToCss implementation, and also quite a lot of time in SharedRwLock::read.

s2

@emilio mentioned in another bug:

We should also look into not serializing the style attribute eagerly when it's
not needed... But a lot of code currently rely on attribute values being
dereferenciables to &str, so that's harder to fix.

Questions:

  1. Is that ToCss call here actually needed? It sounds like @emilio was saying it's calculated here when it isn't actually needed, and could perhaps be done lazily?
  2. Is there anything we can do about the time spent in the lock primitive?

If removing the ToCss call here is feasible, that could potentially halve the cost of our setProperty calls, which would be a massive win in this benchmark.

@glennw

This comment has been minimized.

Copy link
Member Author

glennw commented Jun 19, 2017

cc @emilio @jdm @bholley @SimonSapin since I'm not sure who knows this stuff best.

@glennw

This comment has been minimized.

Copy link
Member Author

glennw commented Jun 19, 2017

Hmmm, I wonder if there is a (relatively) easy way I could hack out the ToCss usage in setProperty, for testing the potential improvement locally?

@glennw

This comment has been minimized.

Copy link
Member Author

glennw commented Jun 19, 2017

Applying the follow diff as a hack changes the frame rate on my machine from 39 to 52 fps in this test.

diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs
index 2ef448720d..bdea3c3877 100644
--- a/components/script/dom/cssstyledeclaration.rs
+++ b/components/script/dom/cssstyledeclaration.rs
@@ -85,8 +85,8 @@ impl CSSStyleOwner {
                     //
                     // [1]: https://github.com/whatwg/html/issues/2306
                     if let Some(pdb) = attr {
-                        let guard = shared_lock.read();
-                        let serialization = pdb.read_with(&guard).to_css_string();
+                        //let guard = shared_lock.read();
+                        let serialization = String::new();//pdb.read_with(&guard).to_css_string();
                         el.set_attribute(&local_name!("style"),
                                          AttrValue::Declaration(serialization,
                                                                 pdb));

@jdm jdm added the I-perf-slow label Jun 19, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 19, 2017

ToCss here is used to update the string backing Element.getAttribute("style") and selectors like [style^=font]. It looks like this is done eagerly whenever a setter of Element.style.something is used. Instead we should have a "dirty bit" and only serialize lazily when we need the actual string. This is tricky in particular for the selector case because we do selector matching on multiple threads, so some form of synchronization is needed. It likely can’t be SharedRwLock::write on a document-wide or process-wide lock because that would conflict (and possibly deadlock) with other threads.

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Jun 19, 2017

I think the right fix for this, in particular for the style attribute is not storing the serialized string anywhere. This also applies for other attribute values, fwiw...

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 19, 2017

Gecko serializes and caches the string on demand, but does not cache when in a servo restyling traversal (i.e. when off-main-thread).

case eCSSDeclaration in void nsAttrValue::ToString(nsAString& aResult) const

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Jun 20, 2017

@emilio are you working on this? If so, do you have a rough timeline? We're hoping to get this fixed for demos at all hands, but we can use the hack to disable serialization as a stop gap for next week if needed.

@bholley

This comment has been minimized.

Copy link
Contributor

bholley commented Jun 20, 2017

I think this may be marginally more up @SimonSapin's alley, given that he's looking into parsing/serialization performance on stylo right now. But it's probably more a question of who has cycles to look at it.

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 20, 2017

@pcwalton, you said on IRC you’d try to remove Deref<Target=str> for AttrValue. How did that go?

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Jun 20, 2017

I started but stopped working on it because @emilio said that he was working on it.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Jun 20, 2017

I was using khuey's lazy-init crate. @emilio, can you explain how you wanted to solve it?

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Jun 21, 2017

My idea was to remove the Deref<Target=str> in all the attributes, as well as the Strings in all of them, and passing the read-only lock to the users of serialize(), that are left only in DOM APIs.

Current WIP is https://github.com/emilio/servo/tree/attrs. That requires implementing parse_plain_attribute for a few attributes, mainly:

lang -> atom
name -> atom
href -> string
<meta content>
encoding -> string
<style media> -> string
<script type> -> string
<script for> -> string
<script event> -> string
<script src> -> string
<script language> -> string
<textarea placeholder> -> string
<button type> -> atom
<input type> -> atom
<object data> -> string
<table cellspacing> -> u32
<tc span> -> string // FIXME
<li type> -> atom
<ol type> -> atom

Using lazy-init is, I guess, a slightly easier fix, but it doesn't feel like the right fix to me, there's no reason we should be storing all those strings in the first place.

glennw pushed a commit to glennw/servo that referenced this issue Jun 21, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 22, 2017

href -> string

Not URL?

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Jun 22, 2017

It gets resolved when appropriate. At least that's how the code works as of right now, and element.href returns a string, not an url, I think.

@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 22, 2017

Oh, <base href> might change :/

bors-servo added a commit that referenced this issue Jun 22, 2017

Auto merge of #17476 - servo:attrs, r=<try>
Serialize style attributes on demand…

… rather than every time that CSSOM `Element.style` is modified. Results are *not* cached in this PR. We could add APIs that work with `&mut AttrValue` rather than `&AttrValue` to add some caching.

CC #17399

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17476)
<!-- Reviewable:end -->
@SimonSapin

This comment has been minimized.

Copy link
Member

SimonSapin commented Jun 22, 2017

My idea was to remove the Deref<Target=str> in all the attributes, as well as the Strings in all of them

I don’t think we can remove String in most variants and preserve the behavior of Element.setAttribute. I’m opened #17476 that only removes String in the AttrValue::Declaration variant.

bors-servo added a commit that referenced this issue Jun 22, 2017

Auto merge of #17476 - servo:attrs, r=<try>
Serialize style attributes on demand…

… rather than every time that CSSOM `Element.style` is modified. Results are *not* cached in this PR. We could add APIs that work with `&mut AttrValue` rather than `&AttrValue` to add some caching.

CC #17399

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17476)
<!-- Reviewable:end -->

bors-servo added a commit that referenced this issue Jun 22, 2017

Auto merge of #17476 - servo:attrs, r=<try>
Serialize style attributes on demand…

… rather than every time that CSSOM `Element.style` is modified. Results are *not* cached in this PR. We could add APIs that work with `&mut AttrValue` rather than `&AttrValue` to add some caching.

CC #17399

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17476)
<!-- Reviewable:end -->

glennw pushed a commit to glennw/servo that referenced this issue Jun 26, 2017

glennw pushed a commit to glennw/servo that referenced this issue Jun 27, 2017

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