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

Cascade: skip duplicated properties before rather than after a virtual call #15745

Merged
merged 9 commits into from Feb 26, 2017

Conversation

Projects
None yet
4 participants
@SimonSapin
Copy link
Member

commented Feb 26, 2017

This is a subset of #15721. It fixes part of #15558, and refactors a couple things along the way.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 26, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/text.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
  • @KiChjang: components/script/dom/cssstyledeclaration.rs
  • @fitzgen: components/script/dom/cssstyledeclaration.rs
  • @emilio: components/style/properties/longhand/text.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/declaration_block.rs, components/style/properties/longhand/box.mako.rs, components/style/properties/properties.mako.rs
@highfive

This comment has been minimized.

Copy link

commented Feb 26, 2017

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@bors-servo r=emilio

This is #15721, without the parts still being discussed.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

📌 Commit f34a2de has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

⌛️ Testing commit f34a2de with merge 009b338...

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

Auto merge of #15745 - servo:hoist, r=emilio
 Cascade: skip duplicated properties before rather than after a virtual call

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

This is a subset of #15721. It fixes part of #15558, and refactors a couple things along the way.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

💔 Test failed - linux-rel-wpt

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

Found the bug. I moved code like if seen.get${maybe_physical}_${property.ident}(${maybe_wm}) to a place that was not in a Mako loop for the property variable. The only reason this compiled is that Python’s variables are scoped to functions rather than blocks, so for-loop variables "leak" after the loop.

This means that the mapping from logical property ID to physical based on the writing mode now needs to be done at runtime. In retrospect this makes sense: we moved this code to before the virtual dispatch to property-specific functions.

This has some runtime cost (a match expression on the LonghandId C-like enum), but hopefully that is compensated by doing fewer virtual calls.

I also did a bunch of drive-by clean-up. (5 last commits)

r? @emilio or @Manishearth

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

⌛️ Trying commit 5dd77e7 with merge a1637ed...

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

Auto merge of #15745 - servo:hoist, r=<try>
 Cascade: skip duplicated properties before rather than after a virtual call

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

This is a subset of #15721. It fixes part of #15558, and refactors a couple things along the way.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

💔 Test failed - linux-dev

SimonSapin added some commits Feb 26, 2017

Remove/don’t call no-op compute_font_hash method for stylo.
This removes a potentially-costly Arc::mut call.
Remove some accessors for public fields.
(Maybe they date from this was a trait?)
@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@bors-servo try

(tidy)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

⌛️ Trying commit d007eef with merge c96cd9c...

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

Auto merge of #15745 - servo:hoist, r=<try>
 Cascade: skip duplicated properties before rather than after a virtual call

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

This is a subset of #15721. It fixes part of #15558, and refactors a couple things along the way.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

@emilio

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

@bors-servo r+

Awesome :)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

📌 Commit d007eef has been approved by emilio

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

📌 Commit d007eef has been approved by emilio

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

My guess is that the merge commit for the last try run of this PR is still ahead of master, so homu thinks there is no need to make a new build. And since there is not successful non-try build, homu doesn’t merge either.

I’ll merge manually on the basis that the earlier try is still valid, since nothing has been merged to master since.

@SimonSapin SimonSapin merged commit 7c5ac06 into master Feb 26, 2017

2 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details

@SimonSapin SimonSapin deleted the hoist branch Feb 26, 2017

@bors-servo bors-servo referenced this pull request Feb 26, 2017

Closed

Don’t deduplicate declarations at parse time #15721

4 of 5 tasks complete
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.