Use a builder for local CascadeLayers [chromium/src : main]
Anders Hartvoll Ruud (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 3 comments![]( "Open in Gerrit")
The plan is to keep the system we have now as much as possible (withBefore I go too deeply into this CL: What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
A related question is: Why do we want the local CascadeLayer tree at all in the future? What is it good for?
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
HeapVector<wtf_size_t> stack_sizes_;Is there some reason why you do this instead of HeapVector<HeapVector<Member<CascadeLayer>, 1>>> or similar?
It also feels more natural to store e.g. 1 here instead of having to subtract the number of elements?
HeapVector<Member<CascadeLayer>> stack_;Where's the root stored?
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 3 comments![]( "Open in Gerrit")
The plan is to keep the system we have now as much as possible (withBefore I go too deeply into this CL: What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
A related question is: Why do we want the local CascadeLayer tree at all in the future? What is it good for?
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
No, but it means the canonical layer tree (and layer->order map) needs to be rebuilt. (Like today.)
Why do we want the local CascadeLayer tree at all in the future? What is it good for?
To maintain the incrementality you're talking about. If we store canonical layers directly, we would have to rebuild all subsequent RuleSets when a sheet is inserted.
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Yeah. The plan is to make `CascadeLayerBuilder` capable of duplicating its push/pops onto a second builder (for the canonical tree and layer map). This second builder is instantiated in `StyleSheetCollection::FinishUpdateActiveStyleSheets`, and either receives push/pops via a local builder when building a `RuleSet`, or, if the cached `RuleSet` held on `StyleSheetContents` is still valid, we basically "replay" the push/pops from the local layer tree stored on the `RuleSet`. As long as we're not doing any layered lookups during `RuleSet` building (custom media, mixins, etc), then it's really very similar to how it works today. We do roughly the same work, just earlier.
HeapVector<wtf_size_t> stack_sizes_;Is there some reason why you do this instead of HeapVector<HeapVector<Member<CascadeLayer>, 1>>> or similar?
It also feels more natural to store e.g. 1 here instead of having to subtract the number of elements?
HeapVector<HeapVector<Member<CascadeLayer>, 1>>>
Do you think that's better? I just flattened it out instinctively. (Another version of this traversed the ancestor chain, which is more annoying with a nested structure.)
Also, I think it's more annoying to map the `foo` part of `foo.bar` from local to canonical if we don't flatten.
Let's come back to this point when the next CL in the chain is ready to look at.
It also feels more natural to store e.g. 1 here instead of having to subtract the number of elements?
Yeah, sure. I'll do that.
HeapVector<Member<CascadeLayer>> stack_;Where's the root stored?
At `stack_[0]`, created lazily. I'll mention it here.
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 3 comments![]( "Open in Gerrit")
The plan is to keep the system we have now as much as possible (withAnders Hartvoll RuudBefore I go too deeply into this CL: What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
A related question is: Why do we want the local CascadeLayer tree at all in the future? What is it good for?
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
No, but it means the canonical layer tree (and layer->order map) needs to be rebuilt. (Like today.)
Why do we want the local CascadeLayer tree at all in the future? What is it good for?
To maintain the incrementality you're talking about. If we store canonical layers directly, we would have to rebuild all subsequent RuleSets when a sheet is inserted.
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Yeah. The plan is to make `CascadeLayerBuilder` capable of duplicating its push/pops onto a second builder (for the canonical tree and layer map). This second builder is instantiated in `StyleSheetCollection::FinishUpdateActiveStyleSheets`, and either receives push/pops via a local builder when building a `RuleSet`, or, if the cached `RuleSet` held on `StyleSheetContents` is still valid, we basically "replay" the push/pops from the local layer tree stored on the `RuleSet`. As long as we're not doing any layered lookups during `RuleSet` building (custom media, mixins, etc), then it's really very similar to how it works today. We do roughly the same work, just earlier.
Hm. But we _are_ going to do layered lookups during RuleSet building for mixins, right?
I'm fine with not solving that here if it isn't intended to be solved yet, I just want to understand to what degree the path is clear for us.
HeapVector<wtf_size_t> stack_sizes_;Anders Hartvoll RuudIs there some reason why you do this instead of HeapVector<HeapVector<Member<CascadeLayer>, 1>>> or similar?
It also feels more natural to store e.g. 1 here instead of having to subtract the number of elements?
HeapVector<HeapVector<Member<CascadeLayer>, 1>>>
Do you think that's better? I just flattened it out instinctively. (Another version of this traversed the ancestor chain, which is more annoying with a nested structure.)
Also, I think it's more annoying to map the `foo` part of `foo.bar` from local to canonical if we don't flatten.
Let's come back to this point when the next CL in the chain is ready to look at.
It also feels more natural to store e.g. 1 here instead of having to subtract the number of elements?
Yeah, sure. I'll do that.
Acknowledged
HeapVector<Member<CascadeLayer>> stack_;Anders Hartvoll RuudWhere's the root stored?
At `stack_[0]`, created lazily. I'll mention it here.
OK. But why do we create it lazily? Isn't eagerly easier?
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 2 comments![]( "Open in Gerrit")
The plan is to keep the system we have now as much as possible (withAnders Hartvoll RuudBefore I go too deeply into this CL: What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
A related question is: Why do we want the local CascadeLayer tree at all in the future? What is it good for?
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Steinar H GundersonWhat does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
No, but it means the canonical layer tree (and layer->order map) needs to be rebuilt. (Like today.)
Why do we want the local CascadeLayer tree at all in the future? What is it good for?
To maintain the incrementality you're talking about. If we store canonical layers directly, we would have to rebuild all subsequent RuleSets when a sheet is inserted.
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Yeah. The plan is to make `CascadeLayerBuilder` capable of duplicating its push/pops onto a second builder (for the canonical tree and layer map). This second builder is instantiated in `StyleSheetCollection::FinishUpdateActiveStyleSheets`, and either receives push/pops via a local builder when building a `RuleSet`, or, if the cached `RuleSet` held on `StyleSheetContents` is still valid, we basically "replay" the push/pops from the local layer tree stored on the `RuleSet`. As long as we're not doing any layered lookups during `RuleSet` building (custom media, mixins, etc), then it's really very similar to how it works today. We do roughly the same work, just earlier.
Hm. But we _are_ going to do layered lookups during RuleSet building for mixins, right?
I'm fine with not solving that here if it isn't intended to be solved yet, I just want to understand to what degree the path is clear for us.
Hm. But we are going to do layered lookups during RuleSet building for mixins, right?
Yeah. Those RuleSets must be marked as "I care about preceding layers", and do need to be rebuilt when something changes earlier. Within the "local+canonical layer" class of solutions, I've considered a few different options for this:
1. Storing the "input" canonical layer tree as of a given RuleSet's creation on StyleSheetContents, and comparing the old tree vs. the new tree when considering a cache RuleSet for reuse later. This requires a copy-on-write layer tree unless we want to copy of a lot full trees. I started on this, but having multiple snapshots of the "canonical" layer tree made things even harder to understand, and requires very annoying "remapping" of the local->canonical map.
2. Similar to (1), but instead store the "input" canonical layer tree as a _serialized string_. It's a caveman approach, but it probably would have worked well enough.
However, I think the most promising idea is:
3. During `RuleSet` building, when we need to answer the question "is this mixin in layer `foo` stronger than the existing mixin in layer `bar`?", we consult the canonical layer map and get the answer. We record this answer (as well as the competing layers) in a vector (or set) of "layer comparisons that matter", similar to how `RuleSet::media_query_set_results_` store media query evaluations that matter. During `EnsureRuleSet`, we then need to check if any of the layer comparisons changed, exactly like we do with `DidMediaQueryResultsChange`. This approach should ensure that we don't rebuild layer-order-dependent RuleSets just because the layer tree changed before us; it would have to be changed in a way that matters for the layers that were actually compared.
HeapVector<Member<CascadeLayer>> stack_;Anders Hartvoll RuudWhere's the root stored?
Steinar H GundersonAt `stack_[0]`, created lazily. I'll mention it here.
OK. But why do we create it lazily? Isn't eagerly easier?
I was just maintaining the existing laziness of the root layer (see `EnsureImplicitOuterLayer()`), without deeply considering what is easier.
We can do it eagerly, but then `CascadeLayerBuilder::TakeRoot()` should probably still return `nullptr` if we're just holding on to an empty root layer? (To not persist more stuff than we need.)
Related details
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 8 comments![]( "Open in Gerrit")
The plan is to keep the system we have now as much as possible (withAnders Hartvoll RuudBefore I go too deeply into this CL: What does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
A related question is: Why do we want the local CascadeLayer tree at all in the future? What is it good for?
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Steinar H GundersonWhat does this mean for incrementality? If I add another stylesheet somewhere that's not the end, does it mean all RuleSets need to be recreated?
No, but it means the canonical layer tree (and layer->order map) needs to be rebuilt. (Like today.)
Why do we want the local CascadeLayer tree at all in the future? What is it good for?
To maintain the incrementality you're talking about. If we store canonical layers directly, we would have to rebuild all subsequent RuleSets when a sheet is inserted.
(I'm sort of suspecting there is an “incremental merge” plan here somehow, but I can't quite grasp it)
Yeah. The plan is to make `CascadeLayerBuilder` capable of duplicating its push/pops onto a second builder (for the canonical tree and layer map). This second builder is instantiated in `StyleSheetCollection::FinishUpdateActiveStyleSheets`, and either receives push/pops via a local builder when building a `RuleSet`, or, if the cached `RuleSet` held on `StyleSheetContents` is still valid, we basically "replay" the push/pops from the local layer tree stored on the `RuleSet`. As long as we're not doing any layered lookups during `RuleSet` building (custom media, mixins, etc), then it's really very similar to how it works today. We do roughly the same work, just earlier.
Anders Hartvoll RuudHm. But we _are_ going to do layered lookups during RuleSet building for mixins, right?
I'm fine with not solving that here if it isn't intended to be solved yet, I just want to understand to what degree the path is clear for us.
Hm. But we are going to do layered lookups during RuleSet building for mixins, right?
Yeah. Those RuleSets must be marked as "I care about preceding layers", and do need to be rebuilt when something changes earlier. Within the "local+canonical layer" class of solutions, I've considered a few different options for this:
1. Storing the "input" canonical layer tree as of a given RuleSet's creation on StyleSheetContents, and comparing the old tree vs. the new tree when considering a cache RuleSet for reuse later. This requires a copy-on-write layer tree unless we want to copy of a lot full trees. I started on this, but having multiple snapshots of the "canonical" layer tree made things even harder to understand, and requires very annoying "remapping" of the local->canonical map.
2. Similar to (1), but instead store the "input" canonical layer tree as a _serialized string_. It's a caveman approach, but it probably would have worked well enough.
However, I think the most promising idea is:3. During `RuleSet` building, when we need to answer the question "is this mixin in layer `foo` stronger than the existing mixin in layer `bar`?", we consult the canonical layer map and get the answer. We record this answer (as well as the competing layers) in a vector (or set) of "layer comparisons that matter", similar to how `RuleSet::media_query_set_results_` store media query evaluations that matter. During `EnsureRuleSet`, we then need to check if any of the layer comparisons changed, exactly like we do with `DidMediaQueryResultsChange`. This approach should ensure that we don't rebuild layer-order-dependent RuleSets just because the layer tree changed before us; it would have to be changed in a way that matters for the layers that were actually compared.
I assume you are aware that with #3, you can support circular references by importing a SAT solver. :-)
// This exists two know how many items to pop from `stack_` during Pop().two -> to
HeapVector<Member<CascadeLayer>> stack_;Anders Hartvoll RuudWhere's the root stored?
Steinar H GundersonAt `stack_[0]`, created lazily. I'll mention it here.
Anders Hartvoll RuudOK. But why do we create it lazily? Isn't eagerly easier?
I was just maintaining the existing laziness of the root layer (see `EnsureImplicitOuterLayer()`), without deeply considering what is easier.
We can do it eagerly, but then `CascadeLayerBuilder::TakeRoot()` should probably still return `nullptr` if we're just holding on to an empty root layer? (To not persist more stuff than we need.)
That's fine by me. I don't feel very strongly about this detail, it just feels simpler to me without having thought deeply about it. But I _do_ think the current CurrentLayer() behavior is weird. CurrentLayer on the root should _either_ be consistent, _or_ CHECK-fail. Or is there some deep reason here?
// Add the current layer *after* its children. If CurrentLayer() is (or wouldPerhaps slightly more deliberate line breaking here would make it easier to read.
for (unsigned i = 0; i < watched_selectors.size(); ++i) {Since you're in this function now anyway, consider:
Please fix this WARNING reported by ClangTidy: check: modernize-loop-convert
use range-based for loop instead (https://clang.l...
check: modernize-loop-convert
use range-based for loop instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/loop-convert.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-loop-convert` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
CascadeLayerBuilder& layer_builder,Is this now an in/out parameter? If so, I believe it should be moved later in the argument list.
layer_builder.Push(import_rule->GetLayerName());Should this be some kind of RAII helper? Or is that overkill?
layer = layer ? layer->FindDirectSubLayer(name_part) : nullptr;Do you need the ternary operator here, or can you just break once layer is nullptr?
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |