Reland "Fix getPropertyValue() and setProperty() behavior for 'all'" [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.css complete.
- CSSPropertyUpdateValue: base median = 25353.128994718958 -> patched median = 25480.101536695685
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17139b40090000
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Related details
- Jinho Bang
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
Adding sesse@ who should be a better reviewer for this.
Related details
- Jinho Bang
- Steinar H Gunderson
- Suyeon Ji
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 1 comment![]( "Open in Gerrit")
Reland "Fix getPropertyValue() and setProperty() behavior for 'all'"Do you want me to review this as a reland with minor changes, or fully from scratch? The latter would be quite a bit more involved.
Related details
- Jinho Bang
- Suyeon Ji
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 11 comments![]( "Open in Gerrit")
using HasAllField = MayHaveLogicalPropertiesField::DefineNextValue<bool, 1>;Do we need to define this for immutable sets? We should be consistent with MayHaveLogicalPropertiesField, I guess.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||I know MayHaveLogicalPropertiesField is already written this way, but I don't think it makes sense to do all this bit manipulation per-property. Shouldn't we just have a bool instead, and set it after the loop? Same below, unless we take it out of the immutable sets. (And same for MayHaveLogicalPropertiesField.)
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {All other code here uses ++i, not i++.
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Why do you start only after all_index? Do you have some sort of guarantee of ordering here?
if (property.IsAffectedByAll() &&This definitely requires some sort of comment.
if constexpr (std::is_same_v<T, CSSPropertyID>) {Why does this not hold for CSSProperty?
if (bits_.get<HasAllField>() &&This needs a comment. What is the logic here?
if (bits_.get<HasAllField>()) [[unlikely]] {Please don't add [[likely]]/[[unlikely]].
if (property_id == CSSPropertyID::kAll ||This needs a comment.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||Isn't this more logically written as
```
if (id == CSSPropertyID::kAll) {
bits_.set<HasAllField>(true);
}
```
? Same below.
But we need a comment saying why “all” figures here in a longhand function, despite being a shorthand in the spec.
Same below.
property_vector_.clear();Should we perhaps call Clear() instead?
Suyeon Ji (Gerrit)
Message from Suyeon Ji![]( "Open in Gerrit")
- Jinho Bang
- Steinar H Gunderson
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 12 comments![]( "Open in Gerrit")
Reland "Fix getPropertyValue() and setProperty() behavior for 'all'"Do you want me to review this as a reland with minor changes, or fully from scratch? The latter would be quite a bit more involved.
Thank you for your detailed review!
using HasAllField = MayHaveLogicalPropertiesField::DefineNextValue<bool, 1>;Do we need to define this for immutable sets? We should be consistent with MayHaveLogicalPropertiesField, I guess.
I'm not sure if this is exactly what you were asking for, but unlike MayHaveLogicalPropertiesField, this is required even for immutable sets, for instance, during SerializeShorthand(). Since Chromium treats the all shorthand as a longhand, handling it at runtime is the intended way to fix this bug and align with the spec.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||I know MayHaveLogicalPropertiesField is already written this way, but I don't think it makes sense to do all this bit manipulation per-property. Shouldn't we just have a bool instead, and set it after the loop? Same below, unless we take it out of the immutable sets. (And same for MayHaveLogicalPropertiesField.)
I updated it to use a bool and set it after the loop as you commented.
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {All other code here uses ++i, not i++.
Done
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Why do you start only after all_index? Do you have some sort of guarantee of ordering here?
The CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]
left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.
Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]
all is removed and re-appended, so both left and right are now before all and both are reset.
This definitely requires some sort of comment.
I added a comment before the `for` loop. Thanks!
if constexpr (std::is_same_v<T, CSSPropertyID>) {Why does this not hold for CSSProperty?
I assume it wasn't needed here because this template is only instantiated for CSSPropertyID, AtRuleDescriptorID, and AtomicString, but not for CSSProperty. Does this align with your understanding?
This needs a comment. What is the logic here?
I added a comment. please take a look again.
Please don't add [[likely]]/[[unlikely]].
I saw a 1.5% performance regression in blink_perf.css/CSSPropertyUpdateValue.html without [[unlikely]] when I ran it on my Mac. Since this code is called so frequently, I'm wondering, is a 1.5% regression small enough to ignore, or will PGO automatically optimize this anyway?
if (property_id == CSSPropertyID::kAll ||Suyeon JiThis needs a comment.
Done
bits_.set<HasAllField>(bits_.get<HasAllField>() ||Isn't this more logically written as
```
if (id == CSSPropertyID::kAll) {
bits_.set<HasAllField>(true);
}
```? Same below.
But we need a comment saying why “all” figures here in a longhand function, despite being a shorthand in the spec.
Same below.
Done, thank you for detailed.
Should we perhaps call Clear() instead?
Done
Related details
- Jinho Bang
- 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 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Suyeon JiWhy do you start only after all_index? Do you have some sort of guarantee of ordering here?
The CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]all is removed and re-appended, so both left and right are now before all and both are reset.
What is this consistent order? Is it documented anywhere? I don't think I see any new ordering code here.
More importantly, I think this is just too subtle. Some properties are removed on insert (and we try to maintain that invariant, seemingly without documenting it) but some are not and must be filtered on get? Is there a good reason why we cannot choose one or the other, consistently? It would seem reasonable to me that set('all', ...) removes the offending elements, so that they also disappear on iteration.
Related details
- Jinho Bang
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Suyeon JiWhy do you start only after all_index? Do you have some sort of guarantee of ordering here?
Steinar H GundersonThe CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]all is removed and re-appended, so both left and right are now before all and both are reset.
What is this consistent order? Is it documented anywhere? I don't think I see any new ordering code here.
More importantly, I think this is just too subtle. Some properties are removed on insert (and we try to maintain that invariant, seemingly without documenting it) but some are not and must be filtered on get? Is there a good reason why we cannot choose one or the other, consistently? It would seem reasonable to me that set('all', ...) removes the offending elements, so that they also disappear on iteration.
Sorry for the delay. I've updated the patch after carefully reviewing your feedback.
In a previous patchset[1], I had implemented this using RemovePropertiesAffectedByAll(). I was initially concerned about potential side effects due to a subtle difference in cascade_expansion_test[2], but since removeProperty("all") already uses the same approach, I followed your suggestion and adopted it here as well.
I may not fully understand your intention, but even with RemovePropertiesAffectedByAll(), the filtering is still necessary to detect when a property set after 'all' overrides it.
```
style.setProperty("all", "revert"); // [all:revert]
style.setProperty("width", "50px"); // [all:revert, width:50px]
style.getPropertyValue("width"); // "50px"
style.getPropertyValue("color"); // "revert"
style.getPropertyValue("all"); // ""
```
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7579493/9
[2] https://chromium-review.googlesource.com/c/chromium/src/+/7579493/9/third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
Related details
- Jinho Bang
- 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 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Let me try to understand what the strategy is. Are you saying that
1) “all” is kept as a longhand, even though it is a shorthand by the standard. (We expand it in the cascade)
2) Properties are always kept in order of appearance (whether that is as written in the stylesheet, or through some CSSOM modification).
3) As an optimization, we filter out all longhand properties that are redundant by later “all” properties, but we cannot do the same filtering by properties set after the last “all”. This affects storage space only, not really performance.
4) When doing get on a longhand (that is affected by “all”), we must see both whether there is a relevant “all” (with a fast path in the negative case by means of a flag) _and_ whether it is overridden by the actual longhand
5) When doing get on a shorthand, we need to do the same operation for each constituent longhand somehow (I haven't tried to figure out the details yet)
Is my understanding right? If so, this needs to be written down in a (class) comment somewhere, because it's not obvious at all. Someone needs to be able to come here in five years and read up on why we are doing all of these things. :-)
Related details
- Jinho Bang
- Suyeon Ji
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. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Thanks for your review. You're right.
I've left comments at the top of CSSPropertyValueSet class.
Please take another look. Thank you.
Related details
- Jinho Bang
- 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. |