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 uplayout: Remove cached thread local context from LayoutContext, and use LayoutContext for assign_inline_sizes() #15417
Conversation
highfive
commented
Feb 7, 2017
|
Heads up! This PR modifies the following files:
|
highfive
commented
Feb 7, 2017
|
|
|
The main concern of this pull request is that it changed how we access thread local layout context. Now we may need to access tls several times for each text fragment, compare to one time for each flow previously. But @pcwalton said tls access is very fast(three instructions), so this may be small. And it simplifies our parallel layout code, so it may have its value. Would love to know how to make this better. Please criticise! |
|
@bors-servo try |
bors-servo
added a commit
that referenced
this pull request
Feb 7, 2017
layout: Remove cached thread local context from LayoutContext, and use LayoutContext for assign_inline_sizes() <!-- Please describe your changes on the following line: --> According to #3069 the cached thread local context is introduced for green threads. Now green threads has gone, and the existence of cache force us to create a `LayoutContext`, an `AssignISizes` and an `AssignBSizes` for each flow, so the pull request tries to remove it. And it also switch `assign_inline_sizes()` to accept a `LayoutContext` parameter, as according to my current design we need to do full layout to some flex items for column flexbox. Part of #14123. --- <!-- 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 - [X] These changes do not require tests because refactoring <!-- 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/15417) <!-- Reviewable:end -->
|
@bors-servo try- clean try retry |
components/layout/context.rs
Outdated
| *r = Some(context.clone()); | ||
| context | ||
| pub fn with_font_context<F, R>(layout_context: &LayoutContext, f: F) -> R | ||
| where F: FnOnce(&mut FontContext) -> R { |
This comment has been minimized.
This comment has been minimized.
components/layout/context.rs
Outdated
| let context = PersistentThreadLocalLayoutContext::new(shared); | ||
| *r = Some(context.clone()); | ||
| context | ||
| pub fn with_font_context<F, R>(layout_context: &LayoutContext, f: F) -> R |
This comment has been minimized.
This comment has been minimized.
emilio
Feb 7, 2017
Member
Maybe rename to with_thread_local_font_context or something, so it's clear what's going on?
|
@bors-servo delegate+ |
|
|
Remove cached thread local context from LayoutContext, use LayoutContext for assign_inline_sizes(), and simplify the parallel flow traversal code.
|
Comments addressed. Thank you @emilio :) @bors-servo r=emilio |
|
|
bors-servo
added a commit
that referenced
this pull request
Feb 8, 2017
layout: Remove cached thread local context from LayoutContext, and use LayoutContext for assign_inline_sizes() <!-- Please describe your changes on the following line: --> According to #3069 the cached thread local context is introduced for green threads. Now green threads has gone, and the existence of cache force us to create a `LayoutContext`, an `AssignISizes` and an `AssignBSizes` for each flow during parallel layout, so the pull request tries to remove it. And it also switch `assign_inline_sizes()` to accept a `LayoutContext` parameter, as according to my current design we need to do full layout to some flex items for column flexbox during assign isize traversal. Part of #14123. --- <!-- 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 - [X] These changes do not require tests because refactoring <!-- 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/15417) <!-- 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.
stshine commentedFeb 7, 2017
•
edited
According to #3069 the cached thread local context is introduced for green threads. Now green threads has gone, and the existence of cache force us to create a
LayoutContext, anAssignISizesand anAssignBSizesfor each flow during parallel layout, so the pull request tries to remove it. And it also switchassign_inline_sizes()to accept aLayoutContextparameter, as according to my current design we need to do full layout to some flex items for column flexbox during assign isize traversal.Part of #14123.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is