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

Lazily compute and cache the classnames of elements in the style sharing cache. #12912

Closed
emilio opened this issue Aug 17, 2016 · 5 comments
Closed

Comments

@emilio
Copy link
Member

@emilio emilio commented Aug 17, 2016

When #12668 lands, we can compute the StyleSharingCandidate class names the same way as we're computing the CommonStyleAffectingAttributes instead of once per test.

Code:

  • components/style/matching.rs

Tests:

  • Not needed, if it passes tests and review, it's fine.
@highfive
Copy link

@highfive highfive commented Aug 17, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@emilio emilio changed the title Lazily compute and cache the classnames of elements. Lazily compute and cache the classnames of elements in the style sharing cache. Aug 17, 2016
@ashrwin
Copy link
Contributor

@ashrwin ashrwin commented Aug 18, 2016

I'd like to work on this :)

@emilio
Copy link
Member Author

@emilio emilio commented Aug 18, 2016

Hi! The mentioned bug landed, so it's yours!

Let me know if you need any help :)

@emilio emilio added the C-assigned label Aug 18, 2016
@ashrwin
Copy link
Contributor

@ashrwin ashrwin commented Aug 20, 2016

Hi,
I need a bit of help on where exactly the changes should go. This is the first time am dealing with Rust code. Thanks :)

@emilio
Copy link
Member Author

@emilio emilio commented Aug 20, 2016

No problem!

So, in the file components/style/matching.rs, we have a function that compares classes between two elements, called have_same_class.

That function currently takes two elements and compares the two vectors. The key point here is that one element comes straight from the cache, and we might be computing its class attribute a lot of times even when we know it shouldn't change.

So what we'd have to do is adding a field to the StyleSharingCandidate struct, something like our current common_style_affecting_attributes, but for classes.

This field should be an Option<Vec<Atom>>, that is, an optional vector of strings (atoms are the optimized representation for commonly used strings in Servo).

We should add this field, and then write have_same_class in the same way have_same_common_style_affecting_attributes is written, that is, something like:

if candidate.class_attribute.is_none() {
    // compute the class attribute
}
return candidate.class_attribute.unwrap() == element_class_attribute

I hope that helps, thanks!

bors-servo added a commit that referenced this issue Aug 21, 2016
Cached element class names in style sharing cache with lazy computation

<!-- Please describe your changes on the following line: -->
Fixes #12912
Added a field to StyleSharingCandidate to lazily compute and cache the class names.

---
<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/12962)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Aug 22, 2016
Cached element class names in style sharing cache with lazy computation

<!-- Please describe your changes on the following line: -->
Fixes #12912
Added a field to StyleSharingCandidate to lazily compute and cache the class names.

---
<!-- 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 #12912 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests.

<!-- 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/12962)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.