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.

Sign up for GitHub

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

Fix a cached style cascade bug that only manifested in sequential mode #12839

Merged
merged 3 commits into from Aug 14, 2016
Merged

Fix a cached style cascade bug that only manifested in sequential mode #12839

merged 3 commits into from Aug 14, 2016

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Aug 12, 2016 •
edited

When copying cached styles, keep the writing_mode up to date.

EDIT: The test is now working. I ran it with the first commit (the actual fix) reverted and it failed.

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/properties.mako.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 12, 2016
@cbrewster
Copy link
Contributor

r? @bholley @emilio or someone else

@highfive highfive assigned bholley and unassigned cbrewster Aug 13, 2016
@emilio
Copy link
Member

emilio commented Aug 13, 2016

Can you elaborate why it only reproduced on sequential mode?

Also, you can pass --binary-arg="-y 1" to run it sequentially, I guess, though the pref you've used might work?

@bholley bholley assigned emilio and unassigned bholley Aug 13, 2016
@notriddle
Copy link
Contributor Author

The pref doesn't work, because Servo doesn't read it. I put it there as a note.

@notriddle
Copy link
Contributor Author

As for why it only shows up in sequential mode, it's because the parallel layout traversal creates the context fresh every time it does a different chunk, while the sequential layout traversal uses the same one the entire time.

@emilio
Copy link
Member

emilio commented Aug 13, 2016

Oh, I see. You might be able to reproduce it in parallel layout adding more than 64 children then, right? That should make all of them going into the same chunk.

@emilio
Copy link
Member

emilio commented Aug 13, 2016 •
edited

That being said, I think that's flacky-ish. We should run a few tests on sequential mode, probably adding the pref to the resources/prefs.json does the trick.

@notriddle
Copy link
Contributor Author

Can I just change it so that layout_threads is a preference instead of an argument? That would change the command syntax (-y1-> --pref layout_threads 1), but it would fix the problem.

@emilio
Copy link
Member

emilio commented Aug 13, 2016

Why not making it a pref and make -y only set that pref?

@notriddle
Copy link
Contributor Author

That probably is a good idea.

@notriddle
Copy link
Contributor Author

@emilio Done.

@notriddle
Copy link
Contributor Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit ef09ec0 with merge 7726f32...

bors-servo pushed a commit that referenced this pull request Aug 13, 2016
Auto merge of #12839 - notriddle:11818_sequential_layout_bug, r=<try>
7726f32 
Fix a cached style cascade bug that only manifested in sequential mode

When copying cached styles, keep the `writing_mode` up to date.

---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11818 (github issue number if applicable).
- [...] There are tests for these changes

NOTE TO THE REVIEWER: This PR contains a test that works, but I need to run the test in sequential mode (couldn't figure out how to make it show up in parallel mode). What do I need to do to make the test harness pass `-y1` to Servo?

<!-- 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/12839)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 13, 2016
@notriddle
Copy link
Contributor Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit ff0ead5 with merge 561d114...

bors-servo pushed a commit that referenced this pull request Aug 13, 2016
Auto merge of #12839 - notriddle:11818_sequential_layout_bug, r=<try>
561d114 
Fix a cached style cascade bug that only manifested in sequential mode

When copying cached styles, keep the `writing_mode` up to date.

---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11818 (github issue number if applicable).
- [...] There are tests for these changes

NOTE TO THE REVIEWER: This PR contains a test that works, but I need to run the test in sequential mode (couldn't figure out how to make it show up in parallel mode). What do I need to do to make the test harness pass `-y1` to Servo?

<!-- 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/12839)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 13, 2016
@notriddle
Copy link
Contributor Author

Replaced the unwrap with expect, and everything else works. This should be ready to review now.

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 7f1297e with merge d185336...

bors-servo pushed a commit that referenced this pull request Aug 13, 2016
Auto merge of #12839 - notriddle:11818_sequential_layout_bug, r=<try>
d185336 
Fix a cached style cascade bug that only manifested in sequential mode

When copying cached styles, keep the `writing_mode` up to date.

---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11818 (github issue number if applicable).
- [X] There are tests for these changes

EDIT: The test is now working. I ran it with the first commit (the actual fix) reverted and it failed.

<!-- 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/12839)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@metajack
Copy link
Contributor

Wow @notriddle nice find!

cc @avadacatavra

@emilio
Copy link
Member

emilio commented Aug 14, 2016

@bors-servo: r+

Good find @notriddle :)

@bors-servo
Copy link
Contributor

📌 Commit 7f1297e has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 14, 2016
@notriddle
Copy link
Contributor Author

@bors-servo clean retry r=emilio

@bors-servo
Copy link
Contributor

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

📌 Commit 7f1297e has been approved by emilio

@bors-servo
Copy link
Contributor

⌛ Testing commit 7f1297e with merge 700bb91...

bors-servo pushed a commit that referenced this pull request Aug 14, 2016
Auto merge of #12839 - notriddle:11818_sequential_layout_bug, r=emilio
700bb91 
Fix a cached style cascade bug that only manifested in sequential mode

When copying cached styles, keep the `writing_mode` up to date.

---

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11818 (github issue number if applicable).
- [X] There are tests for these changes

EDIT: The test is now working. I ran it with the first commit (the actual fix) reverted and it failed.

<!-- 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/12839)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

@bors-servo bors-servo merged commit 7f1297e into servo:master Aug 14, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 14, 2016
@bors-servo bors-servo mentioned this pull request Aug 14, 2016
Merged
3 tasks
@notriddle notriddle deleted the 11818_sequential_layout_bug branch August 14, 2016 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sequential layout bug with (possibly) RTL direction
7 participants
@notriddle @highfive @cbrewster @emilio @bors-servo @metajack @bholley