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

Move children_to_process to layout #13655

Merged
merged 1 commit into from
Oct 9, 2016

Conversation

bholley
Copy link
Contributor

@bholley bholley commented Oct 9, 2016

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.


This change is Reviewable

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.
@highfive
Copy link

highfive commented Oct 9, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/layout_wrapper.rs, components/script_layout_interface/lib.rs

@highfive
Copy link

highfive commented Oct 9, 2016

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 9, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

r? @emilio

@highfive highfive assigned emilio and unassigned KiChjang Oct 9, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

@bors-servo try

bors-servo pushed a commit that referenced this pull request Oct 9, 2016
Move children_to_process to layout

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.
@bors-servo
Copy link
Contributor

⌛ Trying commit c72fffa with merge 5b5bd18...

@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 9, 2016
@highfive
Copy link

highfive commented Oct 9, 2016

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/abspos-replaced-width-margin-000.htm
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  └ Shutting down the Constellation after generating an output file or exit flag specified

@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

@bors-servo try

@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

@bors-servo retry #13605

@bors-servo
Copy link
Contributor

⌛ Trying commit c72fffa with merge f21f1bc...

bors-servo pushed a commit that referenced this pull request Oct 9, 2016
Move children_to_process to layout

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.

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

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 9, 2016
@highfive
Copy link

highfive commented Oct 9, 2016

  ▶ Unexpected subtest result in /html/webappapis/scripting/event-loops/microtask_after_raf.html:
  └ PASS [expected FAIL] Microtask execute immediately after script

@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

@bors-servo retry #13501

@bors-servo
Copy link
Contributor

⌛ Trying commit c72fffa with merge 9e787c9...

bors-servo pushed a commit that referenced this pull request Oct 9, 2016
Move children_to_process to layout

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.

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

💔 Test failed - linux-rel-css

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 9, 2016
@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

@bors-servo retry
#13509

@bors-servo
Copy link
Contributor

⌛ Trying commit c72fffa with merge 70dbfd2...

bors-servo pushed a commit that referenced this pull request Oct 9, 2016
Move children_to_process to layout

We don't need this for Gecko, and it's hard to implement in that case because
there's nowhere obvious to put it (we don't plan to create TSDs for non-dirty
nodes, and non-dirty nodes can have dirty children which require the
children_to_process atomic). There are various solutions here, but punting is
the easiest.

We'll need to rethink this if/when we need to do a bottom-up traversal for
Gecko.

<!-- 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/13655)
<!-- Reviewable:end -->
fn did_process_child(&self) -> isize {
let data = self.get_partial_layout_data().unwrap().borrow();
let old_value = data.parallel.children_to_process.fetch_sub(1, Ordering::Relaxed);
debug_assert!(old_value >= 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you remember why it was an isize instead of usize? In any case r=me

@emilio
Copy link
Member

emilio commented Oct 9, 2016

@bors-servo: r+

We can do the isize -> usize in a followup since it was an existing problem.

@bors-servo
Copy link
Contributor

📌 Commit c72fffa 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. S-tests-failed The changes caused existing tests to fail. labels Oct 9, 2016
@bors-servo
Copy link
Contributor

@bors-servo bors-servo merged commit c72fffa into servo:master Oct 9, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 9, 2016
@jdm
Copy link
Member

jdm commented Oct 9, 2016

@bholley Please file issues for any intermittent test failures that don't already have them. I updated #13509 for the last retry.

@bholley
Copy link
Contributor Author

bholley commented Oct 9, 2016

I think the reason for isize is that underflow on fetch_sub is undefined. See rust-lang/rust#34618

@bholley bholley deleted the children_to_process branch October 30, 2016 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants