Checked Span: Address `blink_perf` suites [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/blink_perf.bindings complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17bdb32e490000
Related details
- Kalvin Lee
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/blink_perf.css complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/120f5582490000
Related details
- Kalvin Lee
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/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10fb3ea6490000
Related details
- Kalvin Lee
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/blink_perf.dom complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12b5b16a490000
Related details
- Kalvin Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
# Exposition on patchset 7
- `blink_perf.dom` improves somewhat. We should be close to within noise, as a new statistically significant regression comes up, while the order of regressions changes. I think I may have to crack open a babysat lldb run to identify any last Checked Spans.
- `blink_perf.layout` improves a lot --- we are down to 8 statistically significant regressions (from 21), none over 2%.
For now, I think I'd like to get this set landed so I can continue iterating without carrying around this large delta.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
Over to Daniel as a Gerrit-suggested reviewer (and apparently indirect owner of just about every file here).
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
I guess I'm OK with this but I have some overall concerns:
1. Let's try to link some non-Google-internal docs from the CL description for additional context for readers. Let's try to address this before landing; I don't think most of this context needs to be internal only.
2. More generally, where `base::unchecked` gets sprinkled in looks quite arbitrary, and it's easy to get copied and pasted to places it doesn't need (and it seems like it's also easy to introduce small performance regressions/hotspots if we're not careful). That doesn't seem great and it seems like something we'll need to think about for long-term maintainability... I think it's one thing if we just needed it in a few spots; it's another thing to have it spread out over 30+ files (so far, and presumably potentially more in the future).
Related details
- Kalvin Lee
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. |
Kalvin Lee (Gerrit)
Kalvin Lee voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kalvin Lee
| Commit-Queue | +2 |
I guess I'm OK with this but I have some overall concerns:
1. Let's try to link some non-Google-internal docs from the CL description for additional context for readers. Let's try to address this before landing; I don't think most of this context needs to be internal only.
2. More generally, where `base::unchecked` gets sprinkled in looks quite arbitrary, and it's easy to get copied and pasted to places it doesn't need (and it seems like it's also easy to introduce small performance regressions/hotspots if we're not careful). That doesn't seem great and it seems like something we'll need to think about for long-term maintainability... I think it's one thing if we just needed it in a few spots; it's another thing to have it spread out over 30+ files (so far, and presumably potentially more in the future).
1. Done --- pasted the original design doc into the public explainer. Please let me know if you want anything else.
1. nuskos@ suggested we could (as a precondition to launch) just whack a check for this into the presubmit in the vein of `RAW_PTR_EXCLUSION` and friends. Hopefully that would mostly mitigate.
Related details
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")
Unreviewed changes
9 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Checked Span: Address `blink_perf` suites
This change doesn't introduce any behavioral change. The context for
this CL is given by:
https://docs.google.com/document/d/1kduT2_nbBMd49_J2K2Jvw9Z6M18itKz6AIS1S7HMocg/edit?tab=t.0
The linked doc also attaches a public version of the original design
doc.
Looking at the alerts that fired when we last tried to enable Checked
Span in release, `blink_perf` suites are strongly affected.
This CL opts out spans in several places, particularly `renderer/core/`.
It's guided by what pprof surfaces when individual `blink_perf` suites
are run.
- M base/containers/vector_buffer.h
- M mojo/core/channel.cc
- M mojo/core/ipcz_driver/mojo_message.cc
- M third_party/blink/renderer/bindings/core/v8/pass_as_span.h
- M third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.cc
- M third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value_fuzzer.cc
- M third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_deserializer.h
- M third_party/blink/renderer/bindings/core/v8/v8_code_cache.h
- M third_party/blink/renderer/bindings/core/v8/v8_wasm_response_extensions.cc
- M third_party/blink/renderer/bindings/modules/v8/v8_context_snapshot_impl.cc
- M third_party/blink/renderer/core/css/css_property_value_set.cc
- M third_party/blink/renderer/core/css/css_property_value_set.h
- M third_party/blink/renderer/core/css/css_variable_data.h
- M third_party/blink/renderer/core/css/parser/css_parser_token.h
- M third_party/blink/renderer/core/css/parser/css_selector_parser.cc
- M third_party/blink/renderer/core/css/parser/css_selector_parser.h
- M third_party/blink/renderer/core/dom/element_data.h
- M third_party/blink/renderer/core/editing/iterators/text_searcher_icu.cc
- M third_party/blink/renderer/core/html/parser/html_stack_item.h
- M third_party/blink/renderer/core/inspector/inspector_session_state.cc
- M third_party/blink/renderer/core/inspector/v8_inspector_string.cc
- M third_party/blink/renderer/core/inspector/v8_inspector_string.h
- M third_party/blink/renderer/core/layout/block_break_token.h
- M third_party/blink/renderer/core/layout/inline/offset_mapping.cc
- M third_party/blink/renderer/core/typed_arrays/array_buffer/array_buffer_contents.cc
- M third_party/blink/renderer/core/typed_arrays/array_buffer/array_buffer_contents.h
- M third_party/blink/renderer/core/typed_arrays/dom_array_buffer_view.h
- M third_party/blink/renderer/core/typed_arrays/dom_typed_array.h
- M third_party/blink/renderer/core/xml/parser/xml_document_parser.cc
- M third_party/blink/renderer/core/xml/xslt_processor_libxslt.cc
- M third_party/blink/renderer/core/xml/xslt_unicode_sort.cc
- M third_party/blink/renderer/platform/bindings/source_location.cc
- M third_party/blink/renderer/platform/wtf/text/atomic_string.cc
- M third_party/blink/renderer/platform/wtf/text/atomic_string.h
- M third_party/blink/renderer/platform/wtf/text/atomic_string_table.cc
- M third_party/blink/renderer/platform/wtf/vector.h
Code-Review: +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |