Closed
Bug 1380877
Opened 7 years ago
Closed 7 years ago
stylo: Trim some fat from the traversal
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
2.18 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
27.09 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
I have some patches to reduce overhead in the traversal. A major piece of to avoid enumerating the children twice (once during preprocessing, and once from the traversal driver).
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9adf7a220ac8fa870f30a25a1ff8c71da65873fa
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5672159642c2a065c3ac819deae3c891a0c85d34
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acb96a706a8e51673edcf810e61d1b785620cace
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5f7de37e1aab73e137ebcafe942b4782567050c
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bddfaea9e0eabb734d0496d2989e346d2deb8549 Finally green. This took way longer than I thought to get right.
Assignee | ||
Comment 6•7 years ago
|
||
Contrary to the comment around its declaration, we do move the SmallVec in the replace call. This fixes that. MozReview-Commit-ID: Bc1qFPpHQ8t
Attachment #8886738 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 7•7 years ago
|
||
In the next patch, we'll move logic to identify the children for traversal into preprocess_children (which will be renamed), and the set_dirty_descendants logic will move along with it. So left as-is, the code here will clobber the flags. MozReview-Commit-ID: 7ZskKWD4QC3
Attachment #8886739 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 8•7 years ago
|
||
MozReview-Commit-ID: DIHXaVNzbFM
Attachment #8886740 -
Flags: review?(emilio+bugs)
Comment 9•7 years ago
|
||
Comment on attachment 8886738 [details] [diff] [review] Part 1 - Use Drains in parallel.rs. v1 Review of attachment 8886738 [details] [diff] [review]: ----------------------------------------------------------------- I guess, though the code was nicer before...
Attachment #8886738 -
Flags: review?(emilio+bugs) → review+
Updated•7 years ago
|
Attachment #8886739 -
Flags: review?(emilio+bugs) → review+
Comment 10•7 years ago
|
||
Comment on attachment 8886740 [details] [diff] [review] Part 3 - Pass a callback to recalc_style_at to avoid traversing children twice. v1 Review of attachment 8886740 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/gecko/traversal.rs @@ +38,3 @@ > { > if node.is_element() { > let el = node.as_element().unwrap(); While you're here, can you change this to: if let Some(el) = node.as_element() { instead? ::: servo/components/style/traversal.rs @@ +117,5 @@ > > /// A DOM Traversal trait, that is used to generically implement styling for > /// Gecko and Servo. > pub trait DomTraversal<E: TElement> : Sync { > /// Process `node` on the way down, before its children have been processed. Please document the new function. @@ +257,5 @@ > debug_assert!(node.is_text_node()); > false > } > > + /// Returns true if traversal is needed for the given element and subtree. Document that the only place parent_data can be None is if there's no parent? @@ +617,5 @@ > + // * This is a reconstruct traversal. > + // * This is a servo non-incremental traversal. > + // > + // Additionally, there are a few scenarios where we avoid traversing the > + // subtree even if the element styles are out of date. These cases are I don't think "the element styles are out of date" is a great description... Maybe just "the children styles"? @@ +778,5 @@ > important_rules_changed > ) > } > > +fn note_children<E, D, F>( I find "note" a very generic name, and it's doing very concrete stuff... Don't have an immediate better suggestion, though I think the previous name is clearer than this (but still not great!)
Attachment #8886740 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9) > Comment on attachment 8886738 [details] [diff] [review] > Part 1 - Use Drains in parallel.rs. v1 > > Review of attachment 8886738 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess, though the code was nicer before... Yeah, fair. I realize now that I could have just kept the old code and emptied the array after borrowing it (rather than doing the replace). But I'd probably still want a try push, and otherwise this is land-ready, so I'm just going to land it. (In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > I find "note" a very generic name, and it's doing very concrete stuff... > Don't have an immediate better suggestion, though I think the previous name > is clearer than this (but still not great!) Well, pre-process isn't really accurate here, because it both preprocesses and passes them to the caller. We can rename it if we think of a better name. Thanks for the reviews!
Assignee | ||
Comment 12•7 years ago
|
||
https://github.com/servo/servo/pull/17741
Assignee | ||
Comment 13•7 years ago
|
||
I had to rebase anyway, so I made this more minimal patch for part 1. I'm going to assume emilio prefers this. :-) MozReview-Commit-ID: 7nzjMwOmszZ
Attachment #8886818 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8886738 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•