spanification: automatically spanify .../renderer/core/css/properties/css_bitset.h etc. [chromium/src : main]
Kenneth Russell (Gerrit)
Kenneth Russell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenneth Russell
| Code-Review | +1 |
1 comment
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Commit-Queue | +1 |
2 comments
This is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
base::span<const uint64_t> chunks_;should this be a raw_span? I think you might have a presubmit failure if you ran the tests.
The other option would be to keep a pair of base::span<const uint64_t>::iterators for current and end, but that could become invalid as well (depends on lifetime)?
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
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")
This is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Also please the Blink style perftest.
chunks_[bit / 64] |= (1ull << (bit % 64));The .data() was added here specifically to avoid regressions introduced by bounds checking. Please don't remove them :-) See https://chromium-review.googlesource.com/c/chromium/src/+/3878346 and https://source.chromium.org/chromium/chromium/src/+/603cb301d4228ff355e2bea96986b49b51ff1a75
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 3 comments![]( "Open in Gerrit")
Steinar H GundersonThis is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Also please the Blink style perftest.
perftests running.
But not sure of the bot to use and the Story for blink_perf.css.
So I chose mac-m1_mini_2020-perf and ChangeStyleShallowTree.html
should this be a raw_span? I think you might have a presubmit failure if you ran the tests.
The other option would be to keep a pair of base::span<const uint64_t>::iterators for current and end, but that could become invalid as well (depends on lifetime)?
I think raw_span should be the choice.
The .data() was added here specifically to avoid regressions introduced by bounds checking. Please don't remove them :-) See https://chromium-review.googlesource.com/c/chromium/src/+/3878346 and https://source.chromium.org/chromium/chromium/src/+/603cb301d4228ff355e2bea96986b49b51ff1a75
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
- Stephen Nusko
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/178dafeb310000
Related details
- Bryan Enrique Gonzalez
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1094caf7310000
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/16d8d22d310000
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Steinar H GundersonThis is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Bryan Enrique GonzalezAlso please the Blink style perftest.
perftests running.
But not sure of the bot to use and the Story for blink_perf.css.
So I chose mac-m1_mini_2020-perf and ChangeStyleShallowTree.html
FWIW, I didn't mean blink_perf.css, but the StyleCalc parts of blink_perf_tests. You cannot run them on Pinpoint (see compare_blink_perf.cc).
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
- Stephen Nusko
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
Steinar H GundersonThis is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Bryan Enrique GonzalezAlso please the Blink style perftest.
Steinar H Gundersonperftests running.
But not sure of the bot to use and the Story for blink_perf.css.
So I chose mac-m1_mini_2020-perf and ChangeStyleShallowTree.html
FWIW, I didn't mean blink_perf.css, but the StyleCalc parts of blink_perf_tests. You cannot run them on Pinpoint (see compare_blink_perf.cc).
Importantly, you'll have to make sure you get a close to official build to fully verify this performance. Reach out over chat to me if you need links/pointers (Daniel did this recently for another CL as well and is more in your timezone).
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
Steinar H GundersonThis is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Bryan Enrique GonzalezAlso please the Blink style perftest.
Steinar H Gundersonperftests running.
But not sure of the bot to use and the Story for blink_perf.css.
So I chose mac-m1_mini_2020-perf and ChangeStyleShallowTree.html
Stephen NuskoFWIW, I didn't mean blink_perf.css, but the StyleCalc parts of blink_perf_tests. You cannot run them on Pinpoint (see compare_blink_perf.cc).
Importantly, you'll have to make sure you get a close to official build to fully verify this performance. Reach out over chat to me if you need links/pointers (Daniel did this recently for another CL as well and is more in your timezone).
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
- Stephen Nusko
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
Steinar H GundersonThis is also blink code which is performance tuned, mind running speedometer3 and jetstream2 on this in pinpoint?
Bryan Enrique GonzalezAlso please the Blink style perftest.
Steinar H Gundersonperftests running.
But not sure of the bot to use and the Story for blink_perf.css.
So I chose mac-m1_mini_2020-perf and ChangeStyleShallowTree.html
Stephen NuskoFWIW, I didn't mean blink_perf.css, but the StyleCalc parts of blink_perf_tests. You cannot run them on Pinpoint (see compare_blink_perf.cc).
Bryan Enrique GonzalezImportantly, you'll have to make sure you get a close to official build to fully verify this performance. Reach out over chat to me if you need links/pointers (Daniel did this recently for another CL as well and is more in your timezone).
blink_perf_tests donde :D
https://docs.google.com/document/d/1WwIylWkeIJsxpLoehA1aVm9u392KWE07ftYjDq1lRgo/edit?usp=sharing&resourcekey=0-mchWAYRjLVZlt_-1uxAV6Q
So I don't know these tests as well as Steinar, but my reading is:
That BlinkStyleParseTime seems to have improved by a tiny amount 5-150us range improvement, so quite small probably minor improvement because it is consistent across all the tests, but so small that it probably more fair to call it neutral.
BlinkStyleInitialCalcTime seems to be neutral to positive, sometimes it is tiny bit worse <150us but sometimes it is 2000us better.
BlinkStyleRecalcTime seems to have improved across all tests but again tiny amounts.
blink_perf_tests this looks like a neutral to positive change performance wise, with added UaF protection (the raw_ptr) and oob checking on the bitset. So a win-win.
[Speedometer](https://pinpoint-dot-chromeperf.appspot.com/job/178dafeb310000) nothing is highlighted as significant on pinpoint, but the only green is +0.0% and the rest lean red, when I download the results and use pinpoint_ci it suggests potentially minor regressions in complex cases:
```
out/linux/pinpoint_ci ~/Downloads/178dafeb310000.csv
👎 TodoMVC-Backbone [ +0.2%, +0.7%]
👎 TodoMVC-JavaScript-ES6-Webpack-Complex-DOM [ +0.0%, +0.7%]
```
[jetstream2](https://pinpoint-dot-chromeperf.appspot.com/job/1094caf7310000) shows only one regression on the "Worst" run for one subtest which since jetstream2 is relatively noisy and the rest looks like a wash doesn't seem concerning.
[blink_perf.css](https://pinpoint-dot-chromeperf.appspot.com/job/17870454b10000) isn't a benchmark I'm familiar with so I'm not sure its confidence levels but nothing is flagged, I've triggered a rerun with 150 runs rather than 10 which is usually to few to get significance.
My take:
The bounds checking is probably 100% fine, the largest change here is actually `raw_span` is a `raw_ptr` under the hood which means we are now tracking and protecting against UaFs as well as well.
In fact this made me go look up the guidance and according to [raw_ptr](https://chromium.googlesource.com/chromium/src/+/HEAD/base/memory/raw_ptr.md#pointers-to-unprotected-memory-performance-optimization) the current guidance is this directory generally shouldn't have them for performance reasons, the only reason this doesn't trigger that alert is raw_span doesn't trip the regex matcher for raw_ptr.
We could look to remove the raw_span and just use a regular span to avoid the raw_ptr for performance reasons, but I'll let Steinar or someone in blink make the trade off given the information above.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
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/17870454b10000
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
blink_perf_tests donde :D
You ran one iteration, not so easy to say much from that :-) But I did a full run myself until the confidence intervals roughly stabilized, and it's within noise (less than 1% either way). So blink_perf_tests is fine. I'm not that surprised, since I would assume that the compiler manages to get read of the bounds checks in this case.
Do we have updated Speedometer3 tests since you took out the .data() changes, to verify that we don't have a regression there?
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
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/142c942cb10000
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
Awesome, thanks a lot for all the help :D
Yep, Speedometer3 tests updated.
pinpoint_ci shows these (minor?) regressions
```
👎 Perf-Dashboard [ +0.0%, +0.6%]
👎 TodoMVC-Preact-Complex-DOM [ +0.0%, +1.4%]
👎 TodoMVC-React-Redux [ +0.1%, +0.8%]
👎 TodoMVC-WebComponents [ +0.1%, +0.8%]
```
Related details
- Daniel Angulo
- Kenneth Russell
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
- Stephen Nusko
Kenneth Russell (Gerrit)
Kenneth Russell added 1 comment![]( "Open in Gerrit")
I didn't catch any of the subtle performance issues in my earlier review, so would like to defer to others.
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
- Stephen Nusko
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
Can you run a speedometer run with base::span instead of base::raw_span as a member field? I suspect that will make speedometer go back to full neutral, Just to check that assumption?
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
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/110ad4a4b10000
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- 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. |
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 1 comment![]( "Open in Gerrit")
Yes, with that change we go back to full neutral.
```
Charts-chartjs [ -0.2%, +0.7%]
Charts-observable-plot [ -0.2%, +0.5%]
Editor-CodeMirror [ -0.2%, +0.8%]
Editor-TipTap [ -0.1%, +0.2%]
NewsSite-Next [ -0.2%, +0.3%]
NewsSite-Nuxt [ -0.1%, +0.3%]
Perf-Dashboard [ -0.3%, +0.4%]
React-Stockcharts-SVG [ -0.2%, +0.5%]
Score [ -0.2%, +0.0%]
TodoMVC-Angular-Complex-DOM [ -0.9%, +0.4%]
TodoMVC-Backbone [ -0.1%, +0.3%]
TodoMVC-JavaScript-ES5 [ -0.4%, +1.3%]
TodoMVC-JavaScript-ES6-Webpack-Complex-DOM [ -0.2%, +0.3%]
TodoMVC-Lit-Complex-DOM [ -0.4%, +0.4%]
TodoMVC-Preact-Complex-DOM [ -0.6%, +0.7%]
TodoMVC-React-Complex-DOM [ -0.1%, +0.3%]
TodoMVC-React-Redux [ -0.0%, +0.4%]
TodoMVC-Svelte-Complex-DOM [ -0.3%, +0.3%]
TodoMVC-Vue [ -0.1%, +0.3%]
TodoMVC-WebComponents [ -0.4%, +0.3%]
TodoMVC-jQuery [ -0.6%, +0.6%]
```
Related details
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Steinar H Gunderson
- Stephen Nusko
Steinar H Gunderson (Gerrit)
Steinar H Gunderson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Code-Review | +1 |
If S3 is back to neutral, I'm happy. Thanks for spending the time. :-)
#include "base/memory/raw_span.h"- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
- Stephen Nusko
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. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
Acknowledged
Can you also quickly run the blink_perf_tests with a bunch of iterations and update the doc, but I suspect it will be similar because likely the benchmark doesn't suffer from raw_ptr to much (as much as speedometer would).
Related details
- Bryan Enrique Gonzalez
- Daniel Angulo
- Roberto Torres
- Sergio Solano
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")
Note: For how to run many iterations, see compare_blink_perf.cc.
But in general, the style perftest is much more sensitive to style changes than Speedometer3 is. Noise floor from compilation is about 1%.
Bryan Enrique Gonzalez (Gerrit)
Bryan Enrique Gonzalez added 2 comments![]( "Open in Gerrit")
Tests run until I noticed no significant change.
You don't need this anymore.
Done.
The only needed are css_property_names.h and span.h
Related details
- Daniel Angulo
- Keishi Hattori
- Roberto Torres
- Sergio Solano
- 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. |