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

Allow style sharing for elements with ids as long as the ID is not being used for styling #17055

Merged
merged 4 commits into from Jun 5, 2017

Conversation

Projects
None yet
7 participants
@bzbarsky
Copy link
Contributor

commented May 26, 2017


  • There are tests for these changes OR
  • These changes do not require tests because the only impact is on performance and memory.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 26, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/sharing/mod.rs, components/style/selector_map.rs, components/style/sharing/checks.rs, components/style/stylist.rs
  • @emilio: components/style/sharing/mod.rs, components/style/selector_map.rs, components/style/sharing/checks.rs, components/style/stylist.rs
@highfive

This comment has been minimized.

Copy link

commented May 26, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio

emilio approved these changes May 26, 2017

Copy link
Member

left a comment

r=me with that bit. The patch you had on IRC seemed fine.

Thanks!

/// Checks whether we have rules for either of the two ids.
#[inline]
pub fn have_rules_for_ids(shared_context: &SharedStyleContext,
element_id: Option<Atom>,

This comment has been minimized.

Copy link
@emilio

emilio May 26, 2017

Member

This could better take a reference for its arguments, as discussed on IRC.

@bzbarsky bzbarsky force-pushed the bzbarsky:sharing-across-ids branch from f5b806d to 0143b79 May 26, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

📌 Commit 0143b79 has been approved by emilio

@highfive highfive assigned emilio and unassigned glennw May 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

⌛️ Testing commit 0143b79 with merge b9fe031...

bors-servo added a commit that referenced this pull request May 26, 2017

Auto merge of #17055 - bzbarsky:sharing-across-ids, r=emilio
Allow style sharing for elements with ids as long as the ID is not being used for styling

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

---
<!-- 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 part of the problem we're seeing in https://bugzilla.mozilla.org/show_bug.cgi?id=1367862

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the only impact is on performance and memory.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/17055)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 26, 2017

💔 Test failed - linux-rel-wpt

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

@bors-servo retry

Looks like at least #16627 intermittently failing.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 27, 2017

⚡️ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-wpt2, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 27, 2017

💔 Test failed - linux-rel-wpt

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 27, 2017

⚡️ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-wpt2, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt1...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 27, 2017

💔 Test failed - mac-rel-wpt1

@jdm

This comment has been minimized.

Copy link
Member

commented May 27, 2017

The filtered log from the filter-intermittents log shows a bunch of tests that used to fail are now reliably passing.

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

Right. That's actually due to a bug this patch introduces. Specifically, with this patch, this testcase:

<!DOCTYPE html>
<style>
  form { color: red; }
  #foo form { color: green }
</style>
<div>
  <form></form>
</div>
<div id="foo">
  <form>This should be green</form>
</div>

fails with servo, or with stylo and STYLO_THREADS=1.

I'm sorting through whether I can construct a similar testcase that fails without this patch.

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

For those following along, some notes:

First, the testcase fails because the patch violates an invariant that the style sharing cache depends on. That invariant goes something like this: "Every simple selector must either trigger revalidation or have its matching result guaranteed by the pre-checks done in test_candidate". With this patch, the pre-checks no longer guarantee matching results for id selectors, because they only check that the id is not used in the rightmost compound selector of a complex selector. So in the testcase above, the two divs share style (because the pre-checks don't prevent them from doing that), and then the two forms end up sharing style (because all pre-checks pass for them, their parents are sharing style, and no revalidation gets done).

Conclusion: if we want to do this optimization in this form, we need to make selectors involving an id revalidation selectors. But we only need to do that if they're not in the rightmost compound selector, really. Not sure how best to do that in our current revalidation setup.

Second, if you reverse the order of those two divs, and add text to the form without an id, you will notice that sharing is no longer happening. That's because relations_are_shareable does not insert an element into the style cache if it was matched by a selector that had an id simple selector somewhere in it. This seems pretty weird to me; in particular it means that a #foo * selector on a page with <html id="foo"> will completely turn off the cache. I think that if we make ids revalidation selectors we can remove this bit.

Still digging into some other weirdness I'm seeing.

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

Oh, and the patch has another bug: it checks element_id both times in have_rules_for_ids. The second check should be for candidate_id. I'll fix that.

@bzbarsky bzbarsky force-pushed the bzbarsky:sharing-across-ids branch from 0143b79 to 7c73a4c May 27, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

@emilio review ping?

That said, I'm not sure how much the visitor bits here will bitrot as a result of https://bugzilla.mozilla.org/show_bug.cgi?id=1370107 and whether I should wait for that to land first...

@bholley

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

Per IRC discussion, that work will not bitrot these patches.

@emilio

emilio approved these changes Jun 5, 2017

Copy link
Member

left a comment

r=me

/// of our rule maps.
#[inline]
pub fn may_have_rules_for_id(&self,
id: &Atom) -> bool {

This comment has been minimized.

Copy link
@emilio

emilio Jun 5, 2017

Member

nit: This fits in 80 chars I think.

//! both selectors targeting element and selectors targeting pseudo-elements.
//! This relies on matching an element against a pseudo-element-targeting
//! selector being a sensible operation that will effectively check whether that
//! element is a matching originating element for the selector.

This comment has been minimized.

Copy link
@emilio

emilio Jun 5, 2017

Member

Thanks for writing this :)

@bzbarsky bzbarsky force-pushed the bzbarsky:sharing-across-ids branch from 8b4ad3a to 9eea2a5 Jun 5, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

📌 Commit 9eea2a5 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

⌛️ Testing commit 9eea2a5 with merge 94371b7...

bors-servo added a commit that referenced this pull request Jun 5, 2017

Auto merge of #17055 - bzbarsky:sharing-across-ids, r=emilio
Allow style sharing for elements with ids as long as the ID is not being used for styling

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

---
<!-- 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 part of the problem we're seeing in https://bugzilla.mozilla.org/show_bug.cgi?id=1367862

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the only impact is on performance and memory.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/17055)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

💔 Test failed - mac-dev-unit

@bzbarsky bzbarsky force-pushed the bzbarsky:sharing-across-ids branch from 9eea2a5 to 4c320a9 Jun 5, 2017

@bzbarsky

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

📌 Commit 4c320a9 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

⌛️ Testing commit 4c320a9 with merge 429db6a...

bors-servo added a commit that referenced this pull request Jun 5, 2017

Auto merge of #17055 - bzbarsky:sharing-across-ids, r=emilio
Allow style sharing for elements with ids as long as the ID is not being used for styling

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

---
<!-- 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 part of the problem we're seeing in https://bugzilla.mozilla.org/show_bug.cgi?id=1367862

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the only impact is on performance and memory.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/17055)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

@bors-servo bors-servo merged commit 4c320a9 into servo:master Jun 5, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.