style: Don't cache styles that got a cache hit on the second pass. #18497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids doing wasted work, at least in the recascade case, and pretty likely in the other as well.
|
Heads up! This PR modifies the following files:
|
|
|
highfive
added
the
S-awaiting-review
heycam
approved these changes
Sep 14, 2017
components/style/traversal.rs
Outdated
| context.thread_local | ||
| .sharing_cache | ||
| .insert_if_possible( | ||
| if !new_styles.primary.0.reused_via_rule_node { |
There was a problem hiding this comment.
Maybe we should do this check inside insert_if_possible instead?
|
Nice, I like that. @bors-servo r=heycam |
|
📌 Commit 158aa0f has been approved by |
highfive
added
S-awaiting-merge
bors-servo
pushed a commit
that referenced
this pull request
Sep 14, 2017
style: Don't cache styles that got a cache hit on the second pass. This avoids doing wasted work, at least in the recascade case, and pretty likely in the other as well. <!-- 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/18497) <!-- Reviewable:end -->
|
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev |
highfive
removed
the
S-awaiting-merge
bholley
added a commit
to bholley/servo
that referenced
this pull request
Sep 16, 2017
It's easy to construct examples where not inserting in those cases causes performance to get worse (for example, any long list of siblings that match the same selectors while having some non-effectual differences in LocalName/Class/Id/etc). And the LRU nature of the cache already does the right thing of pruning non-useful entries. Fixing this causes several hundred more sharing hits on wikipedia. MozReview-Commit-ID: L7W8vcMnHaq
|
This is a great Improvement for the CascadeOnly case, but less obviously so for MatchAndCascade. I don't think the savings of fewer cache insertions is worth the potential decrease in sharing. Opened #18532 to fix this. |
bors-servo
pushed a commit
that referenced
this pull request
Sep 16, 2017
Undo #18497 in the MatchAndCascade case It's easy to construct examples where not inserting in those cases causes performance to get worse (for example, any long list of siblings that match the same selectors while having some non-effectual differences in LocalName/Class/Id/etc). And the LRU nature of the cache already does the right thing of pruning non-useful entries. Fixing this causes several hundred more sharing hits on wikipedia. <!-- 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/18532) <!-- Reviewable:end -->
avadacatavra
pushed a commit
to avadacatavra/servo
that referenced
this pull request
Oct 17, 2017
It's easy to construct examples where not inserting in those cases causes performance to get worse (for example, any long list of siblings that match the same selectors while having some non-effectual differences in LocalName/Class/Id/etc). And the LRU nature of the cache already does the right thing of pruning non-useful entries. Fixing this causes several hundred more sharing hits on wikipedia. MozReview-Commit-ID: L7W8vcMnHaq
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This avoids doing wasted work, at least in the recascade case, and pretty likely
in the other as well.
This change is