cppgc: Add PageBackend::SetPoolNormalPages() to control page pooling [v8/v8 : main]
José Dapena Paz (Gerrit)
José Dapena Paz has uploaded the change for review![]( "Open in Gerrit")
Commit message
cppgc: Add PageBackend::SetPoolNormalPages() to control page pooling
Add the new V8 runtime flag pool_normal_pages, that is set by default
to true (to keep current behavior). But when set to false it allows
to disable the normal-page pool.
The main motivation is allowing to reduce the virtual address space
allocation. Though the pool will usually release the memory of the
pages (releasing them), the address space is never freed. This is
problematic for 32-bits where the address space is more limited. With
this change, embedders can completely disable the page pool.
A secondary motivation is allowing to later implement a policy that
reduces the address space allocation in certain scenarios (i.e. when
we are close to the limit of address space allocation, we can
temporarily disable the page pool).
Internally, introduce the setter method
cppgc::internal::PageBackend::SetPoolNormalPages(bool) that is used
by the runtime flag, but can be also used internally. When pooling is
off, FreeNormalPageMemory() returns pages directly to the OS instead
of keeping them in reserve.
When switching pooling off after the pool has pages already, it also
immediately drains those pages.
SetPoolNormalPages() is guarded by mutex_ so it is safe to call
concurrently with allocation. DrainPooledPages(), which runs under the
same lock, uses NormalPageMemoryPool::DrainAll() to atomically swap the
pool out via std::exchange, avoiding re-entrant locking while iterating.
A private EraseNormalPageMemoryRegion() helper deduplicates the erase
code and checks in both FreeNormalPageMemory() and DrainPooledPages().
Tests added accordingly.
Change diff
Change information
- M src/flags/flag-definitions.h
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc/page-memory.cc
- M src/heap/cppgc/page-memory.h
- M test/unittests/heap/cppgc/page-memory-unittest.cc
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Marja Hölttä
- Michael Lippautz
- Yang Guo
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")
bool pool_normal_pages_ = true;This basically disables the pool. What's the difference to saying --no-memory-pool here which just avoids the pool at all?
Related details
- José Dapena Paz
- Marja Hölttä
- Yang Guo
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
bool pool_normal_pages_ = true;This basically disables the pool. What's the difference to saying --no-memory-pool here which just avoids the pool at all?
This is for the CPPGC pages pool. I think --no-memory-pool has no effect on CPPGC page pool?
Related details
- Marja Hölttä
- Michael Lippautz
- Yang Guo
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 2 comments![]( "Open in Gerrit")
DEFINE_BOOL(pool_normal_pages, true,Since this is really CppHeap related: Let's name this `cppheap_page_pool` and move it to a `cppheap_*` block.
DrainPooledPages();Can you inline `DrainPooledPages()` (and its `DrainAll()`) in here? There's not much reuse that we get here.
In fact, shouldn't this just be `ReleasedPooledPages()`? Also, is `ReleasedPooledPages()` buggy and missing the deletion from `normal_page_memory_regions_`?
Related details
- Anton Bikineev
- José Dapena Paz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 2 comments![]( "Open in Gerrit")
Since this is really CppHeap related: Let's name this `cppheap_page_pool` and move it to a `cppheap_*` block.
Can you inline `DrainPooledPages()` (and its `DrainAll()`) in here? There's not much reuse that we get here.
In fact, shouldn't this just be `ReleasedPooledPages()`? Also, is `ReleasedPooledPages()` buggy and missing the deletion from `normal_page_memory_regions_`?
Release pooled pages is for releasing the backing memory of a page (but the page will still be "booking" address space to be reused). Keeping addresses around is intentional and usually safer (CPPGC heap addresses will only be used for CPPGC).
That's the reason why my patch proposes to have a runtime flag for disabling that. With it we will not use the page pool at all (so new pages will need to be allocated completely). This should only be enabled by embedders, and likely only in 32 bits platforms, where this is actually a problem with keeping virtual address space ranges.
In the future, I plan to have also calls to drain pooled pages in a way that can be used externally. Some ideas:
- When there is address space pressure. This could be enabled by default in 32 bits platforms, so when we pass a threshold we remove the pooled pages.
- Or maybe after a timeout, so we have the pool available in situations with many memory allocations, but they are freed after that situation finishes.
So that's the reason I have an explicit method for that, I expect later to use it externally. And maybe I could enable by default one of these policies. This current patch is for fully opting out of the CPPGC page pool.
The reason there is DrainAll is avoiding requiring access to the internals in NormalPageMemoryPool from PageBackend. Implementation just delegates to the NormalPageMemoryPool to drain the PMR, and then iterates over them to remove them from the normal page memory regions map (that is the one that will cause the effective deletion of the PMR objects).
Related details
- Anton Bikineev
- 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 1 comment![]( "Open in Gerrit")
DrainPooledPages();José Dapena PazCan you inline `DrainPooledPages()` (and its `DrainAll()`) in here? There's not much reuse that we get here.
In fact, shouldn't this just be `ReleasedPooledPages()`? Also, is `ReleasedPooledPages()` buggy and missing the deletion from `normal_page_memory_regions_`?
Release pooled pages is for releasing the backing memory of a page (but the page will still be "booking" address space to be reused). Keeping addresses around is intentional and usually safer (CPPGC heap addresses will only be used for CPPGC).
That's the reason why my patch proposes to have a runtime flag for disabling that. With it we will not use the page pool at all (so new pages will need to be allocated completely). This should only be enabled by embedders, and likely only in 32 bits platforms, where this is actually a problem with keeping virtual address space ranges.
In the future, I plan to have also calls to drain pooled pages in a way that can be used externally. Some ideas:
- When there is address space pressure. This could be enabled by default in 32 bits platforms, so when we pass a threshold we remove the pooled pages.
- Or maybe after a timeout, so we have the pool available in situations with many memory allocations, but they are freed after that situation finishes.
So that's the reason I have an explicit method for that, I expect later to use it externally. And maybe I could enable by default one of these policies. This current patch is for fully opting out of the CPPGC page pool.
The reason there is DrainAll is avoiding requiring access to the internals in NormalPageMemoryPool from PageBackend. Implementation just delegates to the NormalPageMemoryPool to drain the PMR, and then iterates over them to remove them from the normal page memory regions map (that is the one that will cause the effective deletion of the PMR objects).
Release pooled pages is for releasing the backing memory of a page (but the page will still be "booking" address space to be reused). Keeping addresses around is intentional and usually safer (CPPGC heap addresses will only be used for CPPGC).
This is what is implemented but I do think that's a bug. We do release pooled pages under memory pressure which should free up not just the backing memory but also the reservation on this layer. For 32-bit bit this should make pages go back to the OS. For 64-bit this should mean reservations go back to the cage (next layer).
The reservation is otherwise just leaked as it's not found for allocations anymore, leading to OOMs due to virtual address space (or cage space on 64-bit) exhaustion.
Related details
- Anton Bikineev
- José Dapena Paz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 2 comments![]( "Open in Gerrit")
pages (releasing them), the address space is never freed. This isI guess we don't unmap pages for non-cages configurations, which causes this problem? Would it make sense to just fix it instead?
pages (releasing them), the address space is never freed. This is
problematic for 32-bits where the address space is more limited. WithThis should only be a problem for 32-bit archs, not on the 64-bit caged configurations, where the memory is reserved in the cage anyway?
Related details
- José Dapena Paz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bikineev (Gerrit)
Anton Bikineev added 1 comment![]( "Open in Gerrit")
pages (releasing them), the address space is never freed. This isI guess we don't unmap pages for non-cages configurations, which causes this problem? Would it make sense to just fix it instead?
There is a CL that releases the pages entirely (i.e. both decommits and returns the pages back to the cage or the OS): https://chromium-review.googlesource.com/c/v8/v8/+/7600444. Note however that on caged configurations the virtual memory will still be accounted.
Related details
- José Dapena Paz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |