[Memory Coordinator] Add renderer policy using V8 last resort GC [chromium/src : main]
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
1 comment
}From my understanding, the callback is invoked from here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1635;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32 The `v8::GCCallbackFlags` does not include the `kLastResort` flag from `current_gc_flags_` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1621;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32), which would be important to have to know that we're in a last resort GC? It seems that `kGCCallbackFlagCollectAllAvailableGarbage` is set in CollectGarbageOnMemoryPressure() (invoked *not* on memory pressure - not a good reason to invoke memory clients), Heap::HandleExternalMemoryInterrupt() (would be a good place to tell memory clients to release external memory), but it's not set in other places that could be last resort GCs?
Extra context/question for @mlip...@chromium.org: Blink caches strong references to BlinkGC objects (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278). We can release these strong references, if we're told that an OOM may be declared otherwise. Is kLastResort the right flag to watch for that? (it doesn't seem to be plumbed currently, we would need to do that https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278)
Related details
- Daniel Cheng
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
3 comments
raw_ref<ChildMemoryConsumerRegistry> registry_;Maybe explain the lifetime here (or at least in the constructor overload)
// notify consumers that retains references to the v8 heap.Very minor grammar nit: "retain"
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
From my understanding, the callback is invoked from here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1635;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32 The `v8::GCCallbackFlags` does not include the `kLastResort` flag from `current_gc_flags_` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1621;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32), which would be important to have to know that we're in a last resort GC? It seems that `kGCCallbackFlagCollectAllAvailableGarbage` is set in CollectGarbageOnMemoryPressure() (invoked *not* on memory pressure - not a good reason to invoke memory clients), Heap::HandleExternalMemoryInterrupt() (would be a good place to tell memory clients to release external memory), but it's not set in other places that could be last resort GCs?
Extra context/question for @mlip...@chromium.org: Blink caches strong references to BlinkGC objects (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278). We can release these strong references, if we're told that an OOM may be declared otherwise. Is kLastResort the right flag to watch for that? (it doesn't seem to be plumbed currently, we would need to do that https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278)
Hey, I haved added the "last resort" flag in this CL [0] here. So once that CL got rolled into Chromium you should be able to use it. Note that kGCCallbackFlagCollectAllAvailableGarbage is set as well on last resort GCs but is also used in non-"last resort"-GCs as well. So I suppose it would be better here to check for that new flag and only flush this cache on "last resort"-GCs.
0: https://chromium-review.googlesource.com/c/v8/v8/+/6988487
Related details
- Patrick Monette
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette added 6 comments![]( "Open in Gerrit")
LGTM w/nits and once @mlip...@chromium.org is happy
Done
Thanks. I'll wait for the v8 cl to get rolled into chromium to submit
raw_ref<ChildMemoryConsumerRegistry> registry_;Maybe explain the lifetime here (or at least in the constructor overload)
Done
// notify consumers that retains references to the v8 heap.Very minor grammar nit: "retain"
Done
v8::GCType gc_type,Patrick MonetteI think you also need to filter for `GCType::kGCTypeMarkSweepCompact` here as we also emit the signal for other types and you may receive it a few times otherwise.
I've gone with not looking at `gc_type`, and checking the last resort flag instead.
Dominik InführFrom my understanding, the callback is invoked from here: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1635;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32 The `v8::GCCallbackFlags` does not include the `kLastResort` flag from `current_gc_flags_` (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/heap.cc;l=1621;drc=bd4cbddbef633d3b78f3a63c8ab90e098b799a32), which would be important to have to know that we're in a last resort GC? It seems that `kGCCallbackFlagCollectAllAvailableGarbage` is set in CollectGarbageOnMemoryPressure() (invoked *not* on memory pressure - not a good reason to invoke memory clients), Heap::HandleExternalMemoryInterrupt() (would be a good place to tell memory clients to release external memory), but it's not set in other places that could be last resort GCs?
Extra context/question for @mlip...@chromium.org: Blink caches strong references to BlinkGC objects (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278). We can release these strong references, if we're told that an OOM may be declared otherwise. Is kLastResort the right flag to watch for that? (it doesn't seem to be plumbed currently, we would need to do that https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/memory_cache.h;l=216;drc=b225f5b0974d6e411de16c081d1f95ef7d674278)
Hey, I haved added the "last resort" flag in this CL [0] here. So once that CL got rolled into Chromium you should be able to use it. Note that kGCCallbackFlagCollectAllAvailableGarbage is set as well on last resort GCs but is also used in non-"last resort"-GCs as well. So I suppose it would be better here to check for that new flag and only flush this cache on "last resort"-GCs.
0: https://chromium-review.googlesource.com/c/v8/v8/+/6988487
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Monette (Gerrit)
Patrick Monette voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/controller/memory_coordinator/v8_heap_memory_signal_generator.cc
Insertions: 1, Deletions: 1.
@@ -14,7 +14,7 @@
void OnV8HeapLastResortGC(v8::Isolate* isolate,
v8::GCType gc_type,
v8::GCCallbackFlags flags) {
- if (flags & v8::kGCCallbackFlagCollectAllAvailableGarbage) {
+ if (flags & v8::kGCCallbackFlagLastResort) {
blink::Platform::Current()->OnV8HeapLastResortGC();
}
}
```
```
The name of the file: content/renderer/memory_coordinator/renderer_memory_coordinator_policy.h
Insertions: 1, Deletions: 0.
@@ -36,6 +36,7 @@
private:
void OnRestoreLimitTimerFired();
+ // The registry outlives `this`.
raw_ref<ChildMemoryConsumerRegistry> registry_;
base::OneShotTimer restore_limit_timer_;
```
```
The name of the file: content/renderer/memory_coordinator/renderer_memory_coordinator_policy.cc
Insertions: 1, Deletions: 1.
@@ -46,7 +46,7 @@
}
// The V8 heap is full and can't free enough memory. To help the impending GC,
- // notify consumers that retains references to the v8 heap.
+ // notify consumers that retain references to the v8 heap.
for (auto& consumer_info : *registry_) {
if (consumer_info.traits.release_gc_references ==
base::MemoryConsumerTraits::ReleaseGCReferences::kYes) {
```
Change information
[Memory Coordinator] Add renderer policy using V8 last resort GC
This CL adds a renderer-only policy that notifies relevant
base::MemoryConsumers when V8 is running its last resort GC.
This will be used in a follow-up CL to clear strong references in the
MemoryCache, to reduce the number of OOMs.
- M base/memory_coordinator/memory_consumer_registry.h
- M base/memory_coordinator/mock_memory_consumer.cc
- M base/memory_coordinator/mock_memory_consumer.h
- M content/renderer/BUILD.gn
- A content/renderer/memory_coordinator/renderer_memory_coordinator_policy.cc
- A content/renderer/memory_coordinator/renderer_memory_coordinator_policy.h
- A content/renderer/memory_coordinator/renderer_memory_coordinator_policy_unittest.cc
- M content/renderer/renderer_blink_platform_impl.cc
- M content/renderer/renderer_blink_platform_impl.h
- M content/renderer/renderer_main.cc
- M content/test/BUILD.gn
- M testing/variations/fieldtrial_testing_config.json
- M third_party/blink/public/platform/platform.h
- M third_party/blink/renderer/controller/BUILD.gn
- M third_party/blink/renderer/controller/blink_initializer.cc
- A third_party/blink/renderer/controller/memory_coordinator/v8_heap_memory_signal_generator.cc
- A third_party/blink/renderer/controller/memory_coordinator/v8_heap_memory_signal_generator.h
Code-Review: +1 by Daniel Cheng, +1 by Francois Pierre Doray
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |