Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upstylo: Improve restyling performance #12563
Merged
Conversation
highfive
commented
Jul 23, 2016
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jul 23, 2016
|
|
|
@bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
Jul 23, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
|
|
|
I think these are servo incremental layout bugs uncovered by this patch (since right now we relayout way less than before), I'll verify this during the weekend. |
|
What a better way to know? @bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
Jul 23, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
|
|
|
@bors-servo: try It seems servo relies on bubbling up the widths for layout, so it needs to traverse the whole DOM. |
|
|
bors-servo
added a commit
that referenced
this pull request
Jul 23, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
emilio
force-pushed the
emilio:stylo
branch
2 times, most recently
from
20783ef
to
956f09e
|
@bors-servo: try |
bors-servo
added a commit
that referenced
this pull request
Jul 23, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
|
|
|
@bors-servo p=1 |
bors-servo
added a commit
that referenced
this pull request
Jul 27, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
Tests with unexpected results: |
|
|
|
|
|
@bors-servo: retry
|
bors-servo
added a commit
that referenced
this pull request
Jul 27, 2016
stylo: Improve restyling performance This commit adds hooks to the Servo style traversal to avoid traversing all the DOM for every restyle. Additionally it changes the behavior of the dirty flag to be propagated top down, to prevent extra overhead when an element is dirtied. This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary. CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo. r? @bholley <!-- Please describe your changes on the following line: --> --- <!-- 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 <!-- Either: --> - [x] These changes do not require tests because no geckolib tests yet. <!-- 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/12563) <!-- Reviewable:end -->
|
|
emilio
added a commit
to emilio/servo
that referenced
this pull request
Aug 5, 2016
…nimation frames. The script tick ends up only processing JS callbacks related to animation frames, so CSS transitions/animations end up not working as expected. This could have accidentally worked before servo#12563 because we over-restyled, but now this is no longer the case. Other possible way to do it is making a layout reflow with RAF handle CSS animations/transitions too, but that may not work if the reflow ends up being suppressed (that could very well be the case), and we'd need to handle a lot more state in the document, so this solution (assuming it doesn't break try) seems a bit less flacky. Fixes servo#12749.
bors-servo
added a commit
that referenced
this pull request
Aug 5, 2016
compositor: Send animation ticks to layout even if there are script animation frames. <!-- Please describe your changes on the following line: --> --- <!-- 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 #12749 (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. --> The script tick ends up only processing JS callbacks related to animation frames, so CSS transitions/animations end up not working as expected. This could have accidentally worked before #12563 because we over-restyled, but now this is no longer the case. Other possible way to do it is making a layout reflow with RAF handle CSS animations/transitions too, but that may not work if the reflow ends up being suppressed (that could very well be the case), and we'd need to handle a lot more state in the document, so this solution (assuming it doesn't break try) seems a bit less flacky. Missing a test, will add one soon. Fixes #12749.
bors-servo
added a commit
that referenced
this pull request
Aug 8, 2016
compositor: Send animation ticks to layout even if there are script animation frames. <!-- Please describe your changes on the following line: --> --- <!-- 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 #12749 (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. --> The script tick ends up only processing JS callbacks related to animation frames, so CSS transitions/animations end up not working as expected. This could have accidentally worked before #12563 because we over-restyled, but now this is no longer the case. Other possible way to do it is making a layout reflow with RAF handle CSS animations/transitions too, but that may not work if the reflow ends up being suppressed (that could very well be the case), and we'd need to handle a lot more state in the document, so this solution (assuming it doesn't break try) seems a bit less flacky. Missing a test, will add one soon. Fixes #12749. <!-- 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/12751) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
emilio commentedJul 23, 2016
•
edited by larsbergstrom
This commit adds hooks to the Servo style traversal to avoid traversing all the
DOM for every restyle. Additionally it changes the behavior of the dirty flag to
be propagated top down, to prevent extra overhead when an element is dirtied.
This commit doesn't aim to change the behavior on Servo just yet, since Servo does extra job when dirtying the node related with DOM revision counters that might be necessary.
CC @asajeffrey for the DOM revision counters stuff. When a node is dirty, do all its descendants really need to increment the revision counter, or is this an unintended effect? My intuition is that this is hurting performance quite a lot for servo.
r? @bholley
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is