Add a fast path for comparing fields in “transition: all”. [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16461686710000
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/103b3362710000
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15432c9a710000
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12b08701710000
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1218b3fc710000
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12eb0ed6710000
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 4 comments![]( "Open in Gerrit")
{% macro transition_all_diff(computed_style, properties, with_discrete) %}Assuming I'm reading this correctly, it looks like we dig into fields, and then compare in a property-centric way within that (using `CSSPropertyEquality`). That feels more roundabout than it needs to be:
Why Can't We Just(TM):
- Know which properties (CSSPropertyIDs) any given field is associated with (from possibly new information in json5---you may have basically added this already).
- Diff fields "naively" (like DebugDiff), and set property bits defensively (i.e. marking all properties that could be affected by the field).
- Leave `CSSPropertyEquality` calls to the existing place that happens in the animation update.
return;It's risky to assume that we have test coverage here. I'd expect most properties that test transitions do so with a specific property, not via transition:all. When the "all" path and specific path diverge, we effectively no longer test if some specific property can be transitioned correctly via "all".
// it's not used anyway.I'd prefer to not make that assumption here. Maybe refactor to make the property handle optional.
// Very similar to the loop in CalculateTransitionUpdateForStandardProperty(),
// just a bit more streamlined and adding the Has() check.It would be more clean if we could return the `CSSBitset` as a filter, set all bits on the failure paths, and add the `continue`-check to the existing loop.
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 2 comments![]( "Open in Gerrit")
{% macro transition_all_diff(computed_style, properties, with_discrete) %}Assuming I'm reading this correctly, it looks like we dig into fields, and then compare in a property-centric way within that (using `CSSPropertyEquality`). That feels more roundabout than it needs to be:
Why Can't We Just(TM):
- Know which properties (CSSPropertyIDs) any given field is associated with (from possibly new information in json5---you may have basically added this already).
- Diff fields "naively" (like DebugDiff), and set property bits defensively (i.e. marking all properties that could be affected by the field).
- Leave `CSSPropertyEquality` calls to the existing place that happens in the animation update.
Isn't this what this patch is doing already? I'm not sure if I understand the difference (no pun intended). Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar? That sounds just wrong to me, we would be setting a zillion background properties if a pointer changed and nothing about FillLayersEqual would be right. (I believe I did this initially, but had to change to using the equality code because a fair amount of tests were failing.)
// Very similar to the loop in CalculateTransitionUpdateForStandardProperty(),
// just a bit more streamlined and adding the Has() check.It would be more clean if we could return the `CSSBitset` as a filter, set all bits on the failure paths, and add the `continue`-check to the existing loop.
The biggest problem is that the loop is already doing double duty (longhands versus shorthands). If we're making it also handle the bitset case, I believe it becomes even more convoluted. And we'd have to make a fully-set bitset for each longhand property we want to transition?
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 1 comment![]( "Open in Gerrit")
{% macro transition_all_diff(computed_style, properties, with_discrete) %}Steinar H GundersonAssuming I'm reading this correctly, it looks like we dig into fields, and then compare in a property-centric way within that (using `CSSPropertyEquality`). That feels more roundabout than it needs to be:
Why Can't We Just(TM):
- Know which properties (CSSPropertyIDs) any given field is associated with (from possibly new information in json5---you may have basically added this already).
- Diff fields "naively" (like DebugDiff), and set property bits defensively (i.e. marking all properties that could be affected by the field).
- Leave `CSSPropertyEquality` calls to the existing place that happens in the animation update.
Isn't this what this patch is doing already? I'm not sure if I understand the difference (no pun intended). Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar? That sounds just wrong to me, we would be setting a zillion background properties if a pointer changed and nothing about FillLayersEqual would be right. (I believe I did this initially, but had to change to using the equality code because a fair amount of tests were failing.)
Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar?
Yes.
That sounds just wrong to me
Not if the question is "which properties are _possibly_ affected by the field diffs".
zillion background properties
Nine properties, yes.
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 2 comments![]( "Open in Gerrit")
{% macro transition_all_diff(computed_style, properties, with_discrete) %}Steinar H GundersonAssuming I'm reading this correctly, it looks like we dig into fields, and then compare in a property-centric way within that (using `CSSPropertyEquality`). That feels more roundabout than it needs to be:
Why Can't We Just(TM):
- Know which properties (CSSPropertyIDs) any given field is associated with (from possibly new information in json5---you may have basically added this already).
- Diff fields "naively" (like DebugDiff), and set property bits defensively (i.e. marking all properties that could be affected by the field).
- Leave `CSSPropertyEquality` calls to the existing place that happens in the animation update.
Anders Hartvoll RuudIsn't this what this patch is doing already? I'm not sure if I understand the difference (no pun intended). Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar? That sounds just wrong to me, we would be setting a zillion background properties if a pointer changed and nothing about FillLayersEqual would be right. (I believe I did this initially, but had to change to using the equality code because a fair amount of tests were failing.)
Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar?
Yes.
That sounds just wrong to me
Not if the question is "which properties are _possibly_ affected by the field diffs".
zillion background properties
Nine properties, yes.
Done
return;Steinar H GundersonIt's risky to assume that we have test coverage here. I'd expect most properties that test transitions do so with a specific property, not via transition:all. When the "all" path and specific path diverge, we effectively no longer test if some specific property can be transitioned correctly via "all".
From a quick look, we have about 40 different properties that are tested by means of something that is not “transition: all”. Should we try to extend the test coverage by making most of those tests also run an extra time with “transition: all”? Would that be enough?
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
// it's not used anyway.Steinar H GundersonI'd prefer to not make that assumption here. Maybe refactor to make the property handle optional.
Done. (Depends on the previous patch in the chain.)
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 4 comments![]( "Open in Gerrit")
property_.field = field
field.property = property_I think it would be nice if we somehow communicated that this is the _main_ field for a given property, e.g.:
```
property_.main_field = field
field.is_main_field_for_property = property
```
Otherwise, readers e.g. `{% field.property %}` may assume that's always set, or assume a 1:1 relationship between fields and properties.
{% macro transition_all_diff(computed_style, properties, with_discrete) %}Assuming I'm reading this correctly, it looks like we dig into fields, and then compare in a property-centric way within that (using `CSSPropertyEquality`). That feels more roundabout than it needs to be:
Why Can't We Just(TM):
- Know which properties (CSSPropertyIDs) any given field is associated with (from possibly new information in json5---you may have basically added this already).
- Diff fields "naively" (like DebugDiff), and set property bits defensively (i.e. marking all properties that could be affected by the field).
- Leave `CSSPropertyEquality` calls to the existing place that happens in the animation update.
Anders Hartvoll RuudIsn't this what this patch is doing already? I'm not sure if I understand the difference (no pun intended). Is it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar? That sounds just wrong to me, we would be setting a zillion background properties if a pointer changed and nothing about FillLayersEqual would be right. (I believe I did this initially, but had to change to using the equality code because a fair amount of tests were failing.)
Steinar H GundersonIs it that you don't want to call CSSPropertyEquality here but instead rely on base::ValuesEquivalent() or similar?
Yes.
That sounds just wrong to me
Not if the question is "which properties are _possibly_ affected by the field diffs".
zillion background properties
Nine properties, yes.
Done
Thanks, looks great.
indent(2, true)}}nit: Ew, hard tab.
// Very similar to the loop in CalculateTransitionUpdateForStandardProperty(),
// just a bit more streamlined and adding the Has() check.It would be more clean if we could return the `CSSBitset` as a filter, set all bits on the failure paths, and add the `continue`-check to the existing loop.
The biggest problem is that the loop is already doing double duty (longhands versus shorthands). If we're making it also handle the bitset case, I believe it becomes even more convoluted. And we'd have to make a fully-set bitset for each longhand property we want to transition?
Shame. This would be cleaner as a filter avoiding work rather than a separate (duplicate) path.
And we'd have to make a fully-set bitset for each longhand property we want to transition?
You mean for e.g. `transition-property: <not all>`? Yeah. Is that bad? It doesn't necessarily sound that bad.
But OK, disregard this.
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 2 comments![]( "Open in Gerrit")
property_.field = field
field.property = property_I think it would be nice if we somehow communicated that this is the _main_ field for a given property, e.g.:
```
property_.main_field = field
field.is_main_field_for_property = property
```Otherwise, readers e.g. `{% field.property %}` may assume that's always set, or assume a 1:1 relationship between fields and properties.
Not sure if I like the “is_”; it sounds like a bool? (Granted, Jinja likes to play hard and fast with boolification, but…) Can we rename field to main_field but keep “property” as-is? Or “field.main_field_property”?
indent(2, true)}}Steinar H Gundersonnit: Ew, hard tab.
Done
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 1 comment![]( "Open in Gerrit")
return;It's risky to assume that we have test coverage here. I'd expect most properties that test transitions do so with a specific property, not via transition:all. When the "all" path and specific path diverge, we effectively no longer test if some specific property can be transitioned correctly via "all".
From a quick look, we have about 40 different properties that are tested by means of something that is not “transition: all”. Should we try to extend the test coverage by making most of those tests also run an extra time with “transition: all”? Would that be enough?
Can we add a test which checks that both transition-property:X works _and_ transition-property:all works such that it's easy to add new cases to the suite? Then add a Reasonable Subset of properties in there for now, and then we can pretend we're going to make it complete later.
E.g. just something like:
```
let cases = [
// Property name, from-value, to-value, transition at 50%
["width", "100px", "200px", "150px"],
["height", "100px", "200px", "150px"],
["z-index", "100", "200", "150"],
];
```
and then spam test functions from that.
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
return;Steinar H GundersonIt's risky to assume that we have test coverage here. I'd expect most properties that test transitions do so with a specific property, not via transition:all. When the "all" path and specific path diverge, we effectively no longer test if some specific property can be transitioned correctly via "all".
Anders Hartvoll RuudFrom a quick look, we have about 40 different properties that are tested by means of something that is not “transition: all”. Should we try to extend the test coverage by making most of those tests also run an extra time with “transition: all”? Would that be enough?
Can we add a test which checks that both transition-property:X works _and_ transition-property:all works such that it's easy to add new cases to the suite? Then add a Reasonable Subset of properties in there for now, and then we can pretend we're going to make it complete later.
E.g. just something like:
```
let cases = [
// Property name, from-value, to-value, transition at 50%
["width", "100px", "200px", "150px"],
["height", "100px", "200px", "150px"],
["z-index", "100", "200", "150"],
];
```and then spam test functions from that.
Done
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12923260f10000
Related details
- Anders Hartvoll Ruud
- 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 1 comment![]( "Open in Gerrit")
property_.field = field
field.property = property_I think it would be nice if we somehow communicated that this is the _main_ field for a given property, e.g.:
```
property_.main_field = field
field.is_main_field_for_property = property
```Otherwise, readers e.g. `{% field.property %}` may assume that's always set, or assume a 1:1 relationship between fields and properties.
Not sure if I like the “is_”; it sounds like a bool? (Granted, Jinja likes to play hard and fast with boolification, but…) Can we rename field to main_field but keep “property” as-is? Or “field.main_field_property”?
I called it main_field_if_property, sounds OK?
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
property_.field = field
field.property = property_Steinar H GundersonI think it would be nice if we somehow communicated that this is the _main_ field for a given property, e.g.:
```
property_.main_field = field
field.is_main_field_for_property = property
```Otherwise, readers e.g. `{% field.property %}` may assume that's always set, or assume a 1:1 relationship between fields and properties.
Steinar H GundersonNot sure if I like the “is_”; it sounds like a bool? (Granted, Jinja likes to play hard and fast with boolification, but…) Can we rename field to main_field but keep “property” as-is? Or “field.main_field_property”?
I called it main_field_if_property, sounds OK?
Uh, that makes no sense. I mean property_if_main_field.
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 3 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
LGTM. Thanks for the patience on this one.
property_.field = field
field.property = property_Steinar H GundersonI think it would be nice if we somehow communicated that this is the _main_ field for a given property, e.g.:
```
property_.main_field = field
field.is_main_field_for_property = property
```Otherwise, readers e.g. `{% field.property %}` may assume that's always set, or assume a 1:1 relationship between fields and properties.
Steinar H GundersonNot sure if I like the “is_”; it sounds like a bool? (Granted, Jinja likes to play hard and fast with boolification, but…) Can we rename field to main_field but keep “property” as-is? Or “field.main_field_property”?
Steinar H GundersonI called it main_field_if_property, sounds OK?
Uh, that makes no sense. I mean property_if_main_field.
Acknowledged; good point about "is_".
promise_test(async t => {- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Commit-Queue | +2 |
1 comment
promise_test(async t => {I don't think we need a promise here?
The addDiv() requires it, seemingly. (I mostly copied another test.)
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57560.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
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 voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add a fast path for comparing fields in “transition: all”.
If the author writes “transition: all”, we essentially treat that
as a shorthand with 100+ longhands. However, most of the time,
almost all of those longhands will end up early-aborting due to
the old and new properties being the same. Do that comparison
up-front instead; this allows us to skip entire groups if they
are shared, and also allows straight-line comparison code
instead of going through a large switch every time.
Speedometer3 (M1 Pinpoint, LTO but no PGO, significant results
at 99% CI only):
TodoMVC-Preact-Complex-DOM [ -1.3%, -0.0%]
TodoMVC-JavaScript-ES6-Webpack-Complex-DOM [ -1.0%, -0.2%]
NewsSite-Next [ -1.6%, -1.1%]
NewsSite-Nuxt [ -1.6%, -1.3%]
Score [ +0.1%, +0.4%]
- M third_party/blink/renderer/build/scripts/core/css/css_properties.py
- M third_party/blink/renderer/build/scripts/core/style/computed_style_fields.py
- M third_party/blink/renderer/build/scripts/core/style/make_computed_style_base.py
- M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.cc.tmpl
- M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_base.h.tmpl
- M third_party/blink/renderer/build/scripts/templates/fields/field.tmpl
- M third_party/blink/renderer/core/animation/css/css_animations.cc
- M third_party/blink/renderer/core/animation/css/css_animations.h
- M third_party/blink/renderer/core/css/css_properties.json5
- A third_party/blink/web_tests/external/wpt/css/css-transitions/all-interpolates-same-as-explicit-property.html
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57560
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |