Make CustomProperty not refcount its AtomicString. [chromium/src : main]
0 views
Skip to first unread message
Anders Hartvoll Ruud (Gerrit)
Jul 2, 2026, 4:55:52 PM (3 days ago) Jul 2
to Steinar H Gunderson, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Steinar H Gunderson
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 6 (Latest):
Anders Hartvoll Ruud . unresolved
I can't really recommend this. How many mistakes did we see (and make ourselves) in the old/brittle String->CSSTokenizer->CSSParserToken[Range,Stream] setup? A lot.
Should we at least delete some r-value constructors?
Related details
Attention is currently required from:
- Steinar H Gunderson
Submit Requirements:
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)
Jul 3, 2026, 3:07:08 AM (2 days ago) Jul 3
to Anders Hartvoll Ruud, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Anders Hartvoll Ruud
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Anders Hartvoll Ruud . unresolved
I can't really recommend this. How many mistakes did we see (and make ourselves) in the old/brittle String->CSSTokenizer->CSSParserToken[Range,Stream] setup? A lot.
Should we at least delete some r-value constructors?
Steinar H Gunderson
A possible alternative: Take in a pointer instead of a const-reference. We'd never get any implicit conversion or otherwise temporary references that are kept only during the constructor and then discarded.
Related details
Attention is currently required from:
- Anders Hartvoll Ruud
Submit Requirements:
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)
Jul 3, 2026, 5:26:00 AM (2 days ago) Jul 3
to Steinar H Gunderson, Kenneth Rohde Christiansen, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, feature-me...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Steinar H Gunderson
Anders Hartvoll Ruud voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
Steinar H GundersonI can't really recommend this. How many mistakes did we see (and make ourselves) in the old/brittle String->CSSTokenizer->CSSParserToken[Range,Stream] setup? A lot.
Should we at least delete some r-value constructors?
A possible alternative: Take in a pointer instead of a const-reference. We'd never get any implicit conversion or otherwise temporary references that are kept only during the constructor and then discarded.
Attention is currently required from:
- Steinar H Gunderson
Submit Requirements:
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)
Jul 3, 2026, 5:32:55 AM (2 days ago) Jul 3
to Anders Hartvoll Ruud, Kenneth Rohde Christiansen, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, feature-me...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Steinar H Gunderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Commit-Queue | +1 |
Anders Hartvoll Ruud . resolved
Steinar H GundersonI can't really recommend this. How many mistakes did we see (and make ourselves) in the old/brittle String->CSSTokenizer->CSSParserToken[Range,Stream] setup? A lot.
Should we at least delete some r-value constructors?
Anders Hartvoll RuudA possible alternative: Take in a pointer instead of a const-reference. We'd never get any implicit conversion or otherwise temporary references that are kept only during the constructor and then discarded.
Sounds good.
Steinar H Gunderson
It actually caught two issues, so I guess that's good :-)
Related details
Attention set is empty
Submit Requirements:
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)
Jul 3, 2026, 7:13:02 AM (2 days ago) Jul 3
to Anders Hartvoll Ruud, Kenneth Rohde Christiansen, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, feature-me...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Anders Hartvoll Ruud
Steinar H Gunderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Auto-Submit | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Steinar H Gunderson . resolved
Needs a re-stamp since I needed to add & to a bunch of unit tests.
Related details
Attention is currently required from:
- Anders Hartvoll Ruud
Submit Requirements:
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)
Jul 3, 2026, 7:14:15 AM (2 days ago) Jul 3
to Steinar H Gunderson, Kenneth Rohde Christiansen, Olga Gerchikov, Chromium LUCI CQ, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, feature-me...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Attention needed from Steinar H Gunderson
Anders Hartvoll Ruud voted![]( "Open in Gerrit")
Attention is currently required from:
- Steinar H Gunderson
Submit Requirements:
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)
Jul 3, 2026, 7:48:52 AM (2 days ago) Jul 3
to Steinar H Gunderson, Anders Hartvoll Ruud, Kenneth Rohde Christiansen, Olga Gerchikov, chrom...@appspot.gserviceaccount.com, Menard, Alexis, chromium...@chromium.org, feature-me...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, devtools-re...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Make CustomProperty not refcount its AtomicString.
We spend a fair amount of time constructing short-lived CustomProperty
instances (e.g., during resolving the cascade), which take a refcount
on the AtomicString for the property name. Make it contain a reference
instead, since it's already STACK_ALLOCATED and thus cannot live
forever.
Style perftest is neutral, but there's a Speedometer3 win
(M1 Pinpoint, LTO but no PGO, significant results at 99% only):
NewsSite-Next [ -0.4%, -0.1%]
TodoMVC-React-Redux [ -0.4%, -0.1%]
React-Stockcharts-SVG [ -0.5%, -0.1%]
TodoMVC-React-Complex-DOM [ -0.5%, -0.1%]
TodoMVC-Vue [ -0.6%, -0.2%]
TodoMVC-Backbone [ -0.7%, -0.2%]
Score [ +0.0%, +0.3%]
Change-Id: I10425d33a80ca3a95ab4c0e94d8fbfdc1a2c7342
Commit-Queue: Steinar H Gunderson <se...@chromium.org>
Auto-Submit: Steinar H Gunderson <se...@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <and...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1656496}
Files:
- M third_party/blink/renderer/core/animation/animation_utils.cc
- M third_party/blink/renderer/core/animation/css/css_animations.cc
- M third_party/blink/renderer/core/css/computed_style_css_value_mapping.cc
- M third_party/blink/renderer/core/css/container_query_test.cc
- M third_party/blink/renderer/core/css/css_computed_style_declaration.cc
- M third_party/blink/renderer/core/css/cssom/computed_style_property_map.cc
- M third_party/blink/renderer/core/css/cssom/paint_worklet_style_property_map.cc
- M third_party/blink/renderer/core/css/cssom/paint_worklet_style_property_map_test.cc
- M third_party/blink/renderer/core/css/cssom/prepopulated_computed_style_property_map.cc
- M third_party/blink/renderer/core/css/media_query_evaluator.cc
- M third_party/blink/renderer/core/css/properties/css_property_ref.cc
- M third_party/blink/renderer/core/css/properties/css_property_ref.h
- M third_party/blink/renderer/core/css/properties/css_property_ref_test.cc
- M third_party/blink/renderer/core/css/properties/css_property_test.cc
- M third_party/blink/renderer/core/css/properties/longhands/custom_property.cc
- M third_party/blink/renderer/core/css/properties/longhands/custom_property.h
- M third_party/blink/renderer/core/css/properties/longhands/custom_property_test.cc
- M third_party/blink/renderer/core/css/resolver/cascade_expansion-inl.h
- M third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
- M third_party/blink/renderer/core/css/resolver/style_builder.cc
- M third_party/blink/renderer/core/css/resolver/style_cascade.cc
- M third_party/blink/renderer/core/css/resolver/style_cascade_test.cc
- M third_party/blink/renderer/core/css/resolver/style_resolver.cc
- M third_party/blink/renderer/core/css/resolver/style_resolver_test.cc
- M third_party/blink/renderer/core/css/style_engine_test.cc
- M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
- M third_party/blink/renderer/core/inspector/inspector_style_sheet.cc
- M third_party/blink/renderer/core/style/computed_style_test.cc
Change size: L
Delta: 28 files changed, 236 insertions(+), 153 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages