cppgc: Restore page decommitting on Windows [v8/v8 : main]
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
Hi Michael!
I think this resolves the PMF regression discussed at go/blink-gc-regression-m133. Let me know if the change makes sense and/or feel free to take ownership of it.
Thanks.
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
Hi Michael!
I think this resolves the PMF regression discussed at go/blink-gc-regression-m133. Let me know if the change makes sense and/or feel free to take ownership of it.
Thanks.
I think this is too aggressive and not consistent between other memory used in V8.
That said, I see that there's some hooks missing which we should fix. Ideally, we'd even go further an integrate this pool here into the MemoryPool in V8 that we want to clear more often. I filed https://issues.chromium.org/u/1/issues/463098735 to track this.
to skip releasing pooled pages back to the OS entirely.This is the behavior that we want and now use consistently across V8's managed memory.
FreeMemoryHandling::kDiscardWherePossible) {Removing this means that there's no pool anymore for physical pages and we merely keep the reservations alive.
The V8 condition is similarly guarded: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/sweeper.cc;l=913;drc=db569cd7847d479358bd391a18b2c28393803333;bpv=1;bpt=1
Related details
- Francois Pierre Doray
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/171690ca310000
Related details
- Francois Pierre Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 5 comments![]( "Open in Gerrit")
Michael LippautzHi Michael!
I think this resolves the PMF regression discussed at go/blink-gc-regression-m133. Let me know if the change makes sense and/or feel free to take ownership of it.
Thanks.
I think this is too aggressive and not consistent between other memory used in V8.
That said, I see that there's some hooks missing which we should fix. Ideally, we'd even go further an integrate this pool here into the MemoryPool in V8 that we want to clear more often. I filed https://issues.chromium.org/u/1/issues/463098735 to track this.
The latest patch set only releases pooled pages on memory reducing GCs, which was the behavior prior to crrev.com/c/6074477 + crrev.com/c/6088019, I believe. Is that the desired behavior?
You're right that there are opportunities for further improvements. But we should fix the unintentional removal of pool decommit on memory reducing GCs introduced by crrev.com/c/6074477 + crrev.com/c/6088019 in the meantime?
Please take another look, or feel free to chat/GVC.
to skip releasing pooled pages back to the OS entirely.This is the behavior that we want and now use consistently across V8's managed memory.
The desired behavior is to decommit the pool only on memory reducing GCs, right? Not to never decommit it?
FreeMemoryHandling::kDiscardWherePossible) {Removing this means that there's no pool anymore for physical pages and we merely keep the reservations alive.
The V8 condition is similarly guarded: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/sweeper.cc;l=913;drc=db569cd7847d479358bd391a18b2c28393803333;bpv=1;bpt=1
Addressed in latest patch set
heap_.heap()->page_backend()->ReleasePooledPages();This is correct if ReleasePooledPages() decommits pages, which is the case when `decommit_pooled_pages` is true (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/flags/flag-definitions.h;l=1048;drc=b32663066c4707a90f117b579f1993c7b74a0ad1).
Can we remove that flag?
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 5 comments![]( "Open in Gerrit")
Michael LippautzHi Michael!
I think this resolves the PMF regression discussed at go/blink-gc-regression-m133. Let me know if the change makes sense and/or feel free to take ownership of it.
Thanks.
Francois Pierre DorayI think this is too aggressive and not consistent between other memory used in V8.
That said, I see that there's some hooks missing which we should fix. Ideally, we'd even go further an integrate this pool here into the MemoryPool in V8 that we want to clear more often. I filed https://issues.chromium.org/u/1/issues/463098735 to track this.
The latest patch set only releases pooled pages on memory reducing GCs, which was the behavior prior to crrev.com/c/6074477 + crrev.com/c/6088019, I believe. Is that the desired behavior?
You're right that there are opportunities for further improvements. But we should fix the unintentional removal of pool decommit on memory reducing GCs introduced by crrev.com/c/6074477 + crrev.com/c/6088019 in the meantime?
The latest patch set only releases pooled pages on memory reducing GCs, which was the behavior prior to crrev.com/c/6074477 + crrev.com/c/6088019, I believe. Is that the desired behavior?
Yes.
You're right that there are opportunities for further improvements. But we should fix the unintentional removal of pool decommit on memory reducing GCs introduced by crrev.com/c/6074477 + crrev.com/c/6088019 in the meantime?
I think we should fix the unintentional removal in here.
I think the latest PS works.
to skip releasing pooled pages back to the OS entirely.Francois Pierre DorayThis is the behavior that we want and now use consistently across V8's managed memory.
The desired behavior is to decommit the pool only on memory reducing GCs, right? Not to never decommit it?
Yes, only on memory reducing GCs.
heap_.heap()->page_backend()->ReleasePooledPages();This is correct if ReleasePooledPages() decommits pages, which is the case when `decommit_pooled_pages` is true (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/flags/flag-definitions.h;l=1048;drc=b32663066c4707a90f117b579f1993c7b74a0ad1).
Can we remove that flag?
- Francois Pierre Doray
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/10601c59310000
Related details
- Francois Pierre Doray
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/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11ddcbd2310000
Related details
- Francois Pierre Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 4 comments![]( "Open in Gerrit")
Michael LippautzHi Michael!
I think this resolves the PMF regression discussed at go/blink-gc-regression-m133. Let me know if the change makes sense and/or feel free to take ownership of it.
Thanks.
Francois Pierre DorayI think this is too aggressive and not consistent between other memory used in V8.
That said, I see that there's some hooks missing which we should fix. Ideally, we'd even go further an integrate this pool here into the MemoryPool in V8 that we want to clear more often. I filed https://issues.chromium.org/u/1/issues/463098735 to track this.
Michael LippautzThe latest patch set only releases pooled pages on memory reducing GCs, which was the behavior prior to crrev.com/c/6074477 + crrev.com/c/6088019, I believe. Is that the desired behavior?
You're right that there are opportunities for further improvements. But we should fix the unintentional removal of pool decommit on memory reducing GCs introduced by crrev.com/c/6074477 + crrev.com/c/6088019 in the meantime?
The latest patch set only releases pooled pages on memory reducing GCs, which was the behavior prior to crrev.com/c/6074477 + crrev.com/c/6088019, I believe. Is that the desired behavior?
Yes.
You're right that there are opportunities for further improvements. But we should fix the unintentional removal of pool decommit on memory reducing GCs introduced by crrev.com/c/6074477 + crrev.com/c/6088019 in the meantime?
I think we should fix the unintentional removal in here.
Acknowledged
Please take a look. Thanks.
bool ShouldDiscardMemory(Francois Pierre Doraynit: constexpr
Done
heap_.heap()->page_backend()->ReleasePooledPages();Michael LippautzThis is correct if ReleasePooledPages() decommits pages, which is the case when `decommit_pooled_pages` is true (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/flags/flag-definitions.h;l=1048;drc=b32663066c4707a90f117b579f1993c7b74a0ad1).
Can we remove that flag?
Yes, lets also clean up the flag.
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
Related details
- Francois Pierre Doray
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Auto-Submit | +1 |
1 comment
dinfuehr@: I'm not a V8 committer. Please take a look as V8 committer to meet the 2 committers CR+1 requirement. Thanks!
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 1 comment![]( "Open in Gerrit")
+bikineev@ (in replacement of dinfuehr who is OOO): Can you provide CR+1? I'm not a V8 committer so I need a 2nd CR+1 to land this. Thanks a lot!
Related details
- Anton Bikineev
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted![]( "Open in Gerrit")
- Anton Bikineev
- Francois Pierre Doray
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
cppgc: Restore page decommitting on Windows
Recent CLs (crrev.com/c/6074477 and crrev.com/c/6088019) disabled memory
discarding from cppgc on Windows, which was known to be problematic
(go/issues-with-discard-on-windows). However, this change
unintentionally introduced a regression in Committed Memory (Private
Memory Footprint). The root cause is that the call to
`PageBackend::ReleasePooledPages()` at the end of a sweep cycle was
guarded by `free_memory_handling ==
FreeMemoryHandling::kDiscardWherePossible`, which became always false on
Windows. This call does not discard memory, it decommits memory [1], so
it shouldn't have been always inhibited on Windows.
To fix this problem, this CL changes the values in `FreeMemoryHandling`
to `kRetainMemory` and `kReleaseMemory`. `Sweeper::Start()` no longer
overrides `free_memory_handling` based on `CanDiscardMemory()`. Instead,
`Sweeper` guards each discard memory operation with
`free_memory_handling == FreeMemoryHandling::kReleaseMemory` *and*
`CanDiscardMemory()`. The call to `PageBackend::ReleasePooledPages()` at
the end of a sweep cycle is guarded only by `free_memory_handling ==
FreeMemoryHandling::kReleaseMemory` (not by `CanDiscardMemory()`).
This will reduce 99th of total PMF by a few gigabytes
go/uma-blink-gc-regression-timeline-2025
[1] Assuming that `decommit_pooled_pages` flag is true, which is the
default
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/flags/flag-definitions.h;l=1044;drc=db569cd7847d479358bd391a18b2c28393803333
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc/gc-invoker.cc
- M src/heap/cppgc/heap-config.h
- M src/heap/cppgc/heap.cc
- M src/heap/cppgc/object-allocator.cc
- M src/heap/cppgc/sweeper.cc
- M test/unittests/heap/cppgc/heap-statistics-collector-unittest.cc
- M test/unittests/heap/cppgc/tests.h
Code-Review: +1 by Etienne Pierre-Doray, +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anton Bikineev
| Code-Review | +1 |
1 comment
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
weicong yu (Gerrit)
weicong yu added 2 comments![]( "Open in Gerrit")
return Sweeper::CanDiscardMemory() &&```
static constexpr bool CanDiscardMemory() {
#if defined(V8_OS_WIN)
// Discarding memory on Windows does not decommit the memory and does not
// contribute to reduce the memory footprint. On the other hand, these
// calls become expensive the more memory is allocated in the system and
// can result in hangs. Thus, it is better to not discard on Windows.
return false;
#else
return CheckMemoryIsInaccessibleIsNoop();
#endif
}
```
free_memory_handling == FreeMemoryHandling::kReleaseMemory;```
static constexpr bool CanDiscardMemory() {
#if defined(V8_OS_WIN)
// Discarding memory on Windows does not decommit the memory and does not
// contribute to reduce the memory footprint. On the other hand, these
// calls become expensive the more memory is allocated in the system and
// can result in hangs. Thus, it is better to not discard on Windows.
return false;
#else
return CheckMemoryIsInaccessibleIsNoop();
#endif
}
```
On Windows, CanDiscardMemory will always return false, so ShouldDiscardMemory will also always return false. Does this mean that it is still impossible to discard memory on Windows? @fdo...@chromium.org @mlip...@chromium.org
Related details
- Francois Pierre Doray
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
free_memory_handling == FreeMemoryHandling::kReleaseMemory;```
static constexpr bool CanDiscardMemory() {
#if defined(V8_OS_WIN)
// Discarding memory on Windows does not decommit the memory and does not
// contribute to reduce the memory footprint. On the other hand, these
// calls become expensive the more memory is allocated in the system and
// can result in hangs. Thus, it is better to not discard on Windows.
return false;
#else
return CheckMemoryIsInaccessibleIsNoop();
#endif
}
```On Windows, CanDiscardMemory will always return false, so ShouldDiscardMemory will also always return false. Does this mean that it is still impossible to discard memory on Windows? @fdo...@chromium.org @mlip...@chromium.org
There's a performance/memory tradeoff for discarding memory on Windows where discarding causes lots of problems that can even lead to hangs. So, yes, it's disabled there. The feature here is discard sub-V8-page memory, so we are talking about smaller avings.
We do however return memory to the pool and with this CL now properly release memory again.