Skip to content

/ servo

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

Jump to bottom

Style sharing candidate cache code is probably not working as intended. #12534

Closed
emilio opened this issue Jul 21, 2016 · 4 comments
Closed

Style sharing candidate cache code is probably not working as intended. #12534

emilio opened this issue Jul 21, 2016 · 4 comments
Assignees
@emilio
Labels
A-content/css I-perf-slow

Comments

@emilio
Copy link
Member

@emilio emilio commented Jul 21, 2016

We basically end up never sharing in normal websites.

Just tested with the following patch applied:

diff --git a/components/style/dom.rs b/components/style/dom.rs
index ee40e8a..a94ecf2 100644
--- a/components/style/dom.rs
+++ b/components/style/dom.rs
@@ -16,6 +16,7 @@ use selector_impl::{ElementExt, SelectorImplExt};
 use selectors::Element;
 use selectors::matching::DeclarationBlock;
 use sink::Push;
+use std::fmt::Debug;
 use std::ops::BitOr;
 use std::sync::Arc;
 use string_cache::{Atom, Namespace};
@@ -45,7 +46,7 @@ impl OpaqueNode {
     }
 }

-pub trait TRestyleDamage : BitOr<Output=Self> + Copy {
+pub trait TRestyleDamage : BitOr<Output=Self> + Copy + Debug {
     fn compute(old: Option<&Arc<ComputedValues>>, new: &ComputedValues) -> Self;
     fn rebuild_and_reflow() -> Self;
 }
diff --git a/components/style/traversal.rs b/components/style/traversal.rs
index 76f8539..4bc7b2f 100644
--- a/components/style/traversal.rs
+++ b/components/style/traversal.rs
@@ -228,6 +228,7 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
                 }
             }
             StyleSharingResult::StyleWasShared(index, damage) => {
+                panic!("\n\nSharing Style! {}, {:?}", index, damage);
                 style_sharing_candidate_cache.touch(index);
                 node.set_restyle_damage(damage);
             }

And I've never been able to hit this panic within normal browsing.

Just did a test run locally with this patch applied of ./mach test-css, and the results were:

Ran 9977 tests finished in 3659.0 seconds.
  • 9362 ran as expected. 132 tests skipped.
  • 601 tests timed out unexpectedly
  • 14 tests passed unexpectedly

Presumably the unexpected passes are due to font or similar issues. All the 601 tests that crashed (or at least, everyone of them I could see) were svg tests under /css-transforms-1_dev/html/svg-*.

I'll take a look to see if there's something obvious we're doing bad. AIUI the objective of this cache is to share style between huge amounts of siblings, right?

Another experiment, probably more relevant that the CSS test suite. Ideally this file should hit the style sharing candidate cache:

<!doctype html>
<style>
  div { display: block; background: red; margin: 5px; width: 50px; height: 50px; }
</style>
<div></div>
<div></div>
<div></div>
<!-- x500 -->
<div></div>

Yet it doesn't, even without UA stylesheet. Tomorrow I'll probably experiment more.

cc @pcwalton
@emilio
Copy link
Member Author

@emilio emilio commented Jul 25, 2016

I'm looking into this.
@emilio emilio self-assigned this Jul 25, 2016
@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jul 25, 2016

AIUI the objective of this cache is to share style between huge amounts of siblings, right?

@emilio Yes. This has regressed multiple times :(
@emilio
Copy link
Member Author

@emilio emilio commented Jul 26, 2016

I have an alternative design for it that hopefully doesn't block on all
pseudo-classes (or on most applicable declarations that we didn't handle
for what it's worth).

I'll post some update soon :-)

On 07/25/2016 02:56 PM, Patrick Walton wrote:

AIUI the objective of this cache is to share style between huge
amounts of siblings, right?

@emilio https://github.com/emilio Yes. This has regressed multiple
times :(

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12534 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwuvvA-Mp6xSbRSex1UXSlGwNRi8jeks5qZTETgaJpZM4JRZCv.
@emilio
Copy link
Member Author

@emilio emilio commented Jul 26, 2016

@pcwalton: So I gave a shot at the attributes stuff, but that doesn't prevent other whole kind of pathological cases, unfortunately.

What I'm thinking though (I'd want to like your opinion), is the following: What about getting all the rules that might affect two siblings of the same type (that is, those with + and ~ combinators in their rightmost selector, :{first, last, nth}-{child, of-type}, etc? And make the cache optimistic, then reject matching that subset?

Does that sound plausible to you? It seems like would prevent a lot of early-rejections we do to ensure we always work.
emilio added a commit to emilio/servo that referenced this issue Jul 31, 2016
@emilio
[wip] Rewrite the style sharing candidate cache.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
4cbbac3 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
@emilio emilio mentioned this issue Jul 31, 2016
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 31, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
a93bb08 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jul 31, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
729ed50 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 1, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
477db79 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Aug 11, 2016
@emilio
Rewrite the style sharing candidate cache.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
0a7cfa3 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
bors-servo added a commit that referenced this issue Aug 11, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
2d3cc86 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 11, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
486fa24 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Aug 11, 2016
@emilio
Rewrite the style sharing candidate cache.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
4ce3b43 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
bors-servo added a commit that referenced this issue Aug 11, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
8032706 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Aug 11, 2016
@emilio
Rewrite the style sharing candidate cache.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
d89ea67 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
bors-servo added a commit that referenced this issue Aug 12, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
2492854 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 12, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=<try>
Loading status checks…
b93d5e9 
[wip] Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Aug 17, 2016
@emilio
Rewrite the style sharing candidate cache.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
3af774b 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
bors-servo added a commit that referenced this issue Aug 17, 2016
@bors-servo
Auto merge of #12668 - emilio:style-cache, r=SimonSapin,pcwalton
Loading status checks…
ec7efff 
Rewrite the style sharing candidate cache

<!-- Please describe your changes on the following line: -->

See the first commit's description for a bit of background. I'm making the PR in this state just to verify against try I've handled all cases of possibly incorrect sharing and, if not, fix it.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12534

<!-- Either: -->
- [x] There are tests for these changes OR

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/12668)
<!-- Reviewable:end -->
samuknet added a commit to samuknet/servo that referenced this issue Sep 6, 2016
@emilio @samuknet
Rewrite the style sharing candidate cache.
2f12d7b 
The style candidate cache had regressed a few times (see servo#12534), and my
intuition is that being able to disable all style sharing with a single rule in
the page is really unfortunate.

This commit redesigns the style sharing cache in order to be a optimistic cache,
but then reject candidates if they match different sibling-affecting selectors
in the page, for example.

So far the numbers have improved, but not so much as I'd wanted (~10%/20% of
non-incremental restyling time in general). The current implementation is really
dumb though (we recompute and re-match a lot of stuff), so we should be able to
optimise it quite a bit.

I have different ideas for improving it (that may or may not work), apart of the
low-hanging fruit like don't re-matching candidates all the time but I have to
measure the real impact.

Also, I need to verify it against try.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emilio emilio

Labels
A-content/css I-perf-slow
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Rewrite the style sharing candidate cache
2 participants
@pcwalton @emilio
You can’t perform that action at this time.