Revert "[layout] Don't allow copying of ConstraintSpace and make RareData const." [chromium/src : main]
Rubber Stamper (Gerrit)
Message from Rubber Stamper![]( "Open in Gerrit")
The change is not in the configured time window. Rubber Stamper is only allowed to review reverts within 14 day(s). Learn more: go/rubber-stamper-user-guide.
Related details
- Ian Kilpatrick
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Very strange - I see a decrease with the original patch applied:
On a M4 machine
`./out/Release/blink_perf_tests --gtest_filter=StyleCalcPerfTest.Encyclopedia`
`gn args out/Release`
```
is_debug = false
use_remoteexec = true
enable_profiling = true
```
ToT:
```
BlinkStyleGCAllocated: Encyclopedia= 61758 kB
BlinkStyleGCAllocated: Encyclopedia= 61449 kB
BlinkStyleGCAllocated: Encyclopedia= 61460 kB
BlinkStyleGCAllocated: Encyclopedia= 61456 kB
BlinkStyleGCAllocated: Encyclopedia= 61470 kB
```
With Revert:
```
BlinkStyleGCAllocated: Encyclopedia= 69716 kB
BlinkStyleGCAllocated: Encyclopedia= 69770 kB
BlinkStyleGCAllocated: Encyclopedia= 69776 kB
BlinkStyleGCAllocated: Encyclopedia= 69774 kB
BlinkStyleGCAllocated: Encyclopedia= 69767 kB
```
What were you seeing?
Related details
- Kurt Catti-Schmidt
- 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")
Very strange - I see a decrease with the original patch applied:
On a M4 machine
`./out/Release/blink_perf_tests --gtest_filter=StyleCalcPerfTest.Encyclopedia`
`gn args out/Release`
```
is_debug = false
use_remoteexec = true
enable_profiling = true
```ToT:
```
BlinkStyleGCAllocated: Encyclopedia= 61758 kB
BlinkStyleGCAllocated: Encyclopedia= 61449 kB
BlinkStyleGCAllocated: Encyclopedia= 61460 kB
BlinkStyleGCAllocated: Encyclopedia= 61456 kB
BlinkStyleGCAllocated: Encyclopedia= 61470 kB
```With Revert:
```
BlinkStyleGCAllocated: Encyclopedia= 69716 kB
BlinkStyleGCAllocated: Encyclopedia= 69770 kB
BlinkStyleGCAllocated: Encyclopedia= 69776 kB
BlinkStyleGCAllocated: Encyclopedia= 69774 kB
BlinkStyleGCAllocated: Encyclopedia= 69767 kB
```What were you seeing?
A change from 19472 to 95428 kB. Most other tests are either 5–6 or 15 MB, FWIW. These are the flags I'm using:
```
use_remoteexec = true
use_siso = false
is_official_build = true
chrome_pgo_phase = 0
dcheck_always_on = false
symbol_level = 1
blink_symbol_level = 2
treat_warnings_as_errors = false
```
These Oilpan counters are sometimes weird; we do ask for a complete GC before counting, but there are anomalies sometimes anyway. But 69 MB sounds like a _lot_ in any case, and if there's supposed to be no functional change, why are they dropping by 8 MB (!!) on macOS?
What are you seeing on the other ones? Note that you'll need to run them one at a time to get isolation; I typically do
```
for X in Video Extension News ECommerce Social1 Social2 Encyclopedia Sports Search; do echo -n "$X: "; ./out/Release/blink_perf_tnews --gtest_filter=\*.$X 2>&1 | grep GCAlloc ;done
```
You can find some recent measurements in https://chromium-review.googlesource.com/c/chromium/src/+/7254563 .
Related details
- Ian Kilpatrick
- Kurt Catti-Schmidt
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
No significant change with the other subtests - just Encyclopedia.
ToT:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13947 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27554 kB
News: *RESULT BlinkStyleGCAllocated: News= 7174 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3872 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7219 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2565 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 61442 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18133 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3342 kB
```
With Revert:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13947 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27558 kB
News: *RESULT BlinkStyleGCAllocated: News= 7174 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3892 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7221 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2562 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 69766 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18135 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3342 kB
```
"There should be no behaviour change"
By this I typically mean there should be no web-developer exposed behaviour change (no bug fixes, implementation changes, etc).
why are they dropping by 8 MB (!!) on macOS?
The change which would be doing this is the removal of the copy of the RareData in the `ConstraintSpace` constructor, e.g. `MakeGarbageCollected<RareData>(*other.rare_data_)`
I wonder if the deletion of the operator= switch the exclusion_space_ to using the copy instead of move ctor (can you try https://chromium-review.googlesource.com/c/chromium/src/+/7403005 locally and see if that affects it?).
GCAlloc just measures the bytes allocated not the actual used memory right (e.g. additional fragmentation memory wise should affect this result?)
Related details
- Kurt Catti-Schmidt
- 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")
OK, so most of the others are in the same ballpark across platforms, thankfully. It's only Encylopedia (a Wikipedia page) that's off the charts big on macOS.
I wonder if the deletion of the operator= switch the exclusion_space_ to using the copy instead of move ctor (can you try https://chromium-review.googlesource.com/c/chromium/src/+/7403005 locally and see if that affects it?).
No change; just some noise to the tune of +/- 100 kB.
GCAlloc just measures the bytes allocated not the actual used memory right (e.g. additional fragmentation memory wise should affect this result?)
I believe so, but my understanding of Oilpan internals are limited. I just use the values they give me :-) It _is_ possible that this is some sort of measurement mirage, but if so, it's the first time I've seen something so large.
In support of the “the measurement is off” theory, the max RSS of the benchmark increases from ~180 MB to ~195 MB with the revert.
Related details
- Ian Kilpatrick
- Kurt Catti-Schmidt
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
The RareData patch did progress system_health.memory_desktop on a subbenchmark that is heavy on RareData, see:
https://chrome-perf.corp.goog/u/?rev=1561489
By my reading of how gc_allocated_bytes is caclulated - it might be sensitive to if/when various parts of GC run.
E.g. `StatsCollector::NotifyMarkingCompleted` resets the counter when marking is completed, (presumably just the the alive bytes).
Reading the values only looks valid(?) if `StatsCollector::NotifySafePointForTesting` is called?
cc/ Michael - we are trying to understand the behaviour of `blink::ProcessHeap::TotalAllocatedObjectSize` within:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/css/style_perftest.cc;l=181-241;drc=f1b6d6ff322093d1e36b55345d9bc13a1b4e435c
Whats the safest way to read this value?
Related details
- Kurt Catti-Schmidt
- 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")
FWIW, we call ThreadState::Current()->CollectAllGarbageForTesting() before the measure point, but we don't _afterwards_ (since we want to have some idea of how much garbage we're actually creating). We're not calling NotifySafePointForTesting(), though; perhaps we should do that?
Related details
- Ian Kilpatrick
- Kurt Catti-Schmidt
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")
I hacked up the counter to simply never subtract on GCs or deallocations (so it just counts the number of allocated bytes ever instead), and the picture is still the same: The amount of memory for Encyclopedia goes from 137914 kB to 167627 kB, i.e., a rather large increase. (That's a lot of garbage, in any case!) Extension is also a megabyte or two, the rest are minor.
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
I hacked up the counter to simply never subtract on GCs or deallocations (so it just counts the number of allocated bytes ever instead), and the picture is still the same: The amount of memory for Encyclopedia goes from 137914 kB to 167627 kB, i.e., a rather large increase. (That's a lot of garbage, in any case!) Extension is also a megabyte or two, the rest are minor.
And if I add a forced GC right before measuring instead, I get 17180 kB both before and after. So this change doesn't allocate more permanent memory, but it does seem to me to make more garbage.
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 2 comments![]( "Open in Gerrit")
+r Michael to make sure he sees this.
Steinar H GundersonI hacked up the counter to simply never subtract on GCs or deallocations (so it just counts the number of allocated bytes ever instead), and the picture is still the same: The amount of memory for Encyclopedia goes from 137914 kB to 167627 kB, i.e., a rather large increase. (That's a lot of garbage, in any case!) Extension is also a megabyte or two, the rest are minor.
And if I add a forced GC right before measuring instead, I get 17180 kB both before and after. So this change doesn't allocate more permanent memory, but it does seem to me to make more garbage.
I think there is a sneaky GC happening which is affecting the results.
E.g. I added a:
```
--- a/third_party/blink/renderer/core/css/style_perftest.cc
+++ b/third_party/blink/renderer/core/css/style_perftest.cc
@@ -180,6 +180,8 @@ static StylePerfResult MeasureStyleForDumpedPage(
// only (e.g. --gtest_filter=StyleCalcPerfTest.Video).
ThreadState::Current()->CollectAllGarbageForTesting();
+ HeapAllocator::EnterGCForbiddenScope();
+
size_t orig_gc_allocated_bytes =
blink::ProcessHeap::TotalAllocatedObjectSize();
size_t orig_partition_allocated_bytes =
```
To prevent GC just after the `CollectAllGarbageForTesting` and got:
ToT:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 20662 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 97298 kB
News: *RESULT BlinkStyleGCAllocated: News= 17557 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 10727 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 14551 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 8393 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 121215 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 23942 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 8600 kB
```
With revert:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 20659 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 99941 kB
News: *RESULT BlinkStyleGCAllocated: News= 17557 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 10728 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 14680 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 8393 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 131281 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 24052 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 8714 kB
```
(revert is worse in all cases).
Without the `EnterGCForbiddenScope` I currently get:
ToT:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13882 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27529 kB
News: *RESULT BlinkStyleGCAllocated: News= 7126 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3847 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7184 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2548 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 60773 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18047 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3330 kB
```
With revert:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13882 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27530 kB
News: *RESULT BlinkStyleGCAllocated: News= 7126 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3847 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7188 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2547 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 70048 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18048 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3339 kB
```
So the GC is definitely doing *something* while the test is being run.
I also spun up a linux cloudtop to see if I could reproduce and got similar regression with the revert (I can post numbers later tonight).
Related details
- Kurt Catti-Schmidt
- Michael Lippautz
- 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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Linux cloudtop
ToT:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13896 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27108 kB
News: *RESULT BlinkStyleGCAllocated: News= 7117 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3777 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7135 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2546 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 73224 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18012 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3315 kB
```
With revert:
```
Video: *RESULT BlinkStyleGCAllocated: Video= 13895 kB
Extension: *RESULT BlinkStyleGCAllocated: Extension= 27110 kB
News: *RESULT BlinkStyleGCAllocated: News= 7117 kB
ECommerce: *RESULT BlinkStyleGCAllocated: ECommerce= 3777 kB
Social1: *RESULT BlinkStyleGCAllocated: Social1= 7138 kB
Social2: *RESULT BlinkStyleGCAllocated: Social2= 2546 kB
Encyclopedia: *RESULT BlinkStyleGCAllocated: Encyclopedia= 83174 kB
Sports: *RESULT BlinkStyleGCAllocated: Sports= 18004 kB
Search: *RESULT BlinkStyleGCAllocated: Search= 3320 kB
```