[heap] Limit Liftoff code flushing to last resort GCs [v8/v8 : main]
0 views
Skip to first unread message
Dominik Inführ (Gerrit)
Dec 9, 2025, 9:15:26 AM12/9/25
to Omer Katz, Manos Koukoutos, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Manos Koukoutos and Omer Katz
Dominik Inführ added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File src/heap/heap.cc
Line 4315, Patchset 2 (Latest):
gc_reason == GarbageCollectionReason::kMemoryPressure;Dominik Inführ . unresolved
We could also revisit whether we want to flush on memory pressure GCs. I guess it makes sense but I don't have a strong opinion here. I would keep it as-is for now until it becomes a problem.
Also afaiu we don't do this for JS but maybe we should.
Related details
Attention is currently required from:
- Manos Koukoutos
- Omer Katz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Dec 9, 2025, 9:29:41 AM12/9/25
to Dominik Inführ, Omer Katz, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ and Omer Katz
Manos Koukoutos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Manos Koukoutos
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Dominik Inführ
- Omer Katz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Dec 9, 2025, 9:48:17 AM12/9/25
to Dominik Inführ, Manos Koukoutos, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Omer Katz added 1 comment![]( "Open in Gerrit")
File src/heap/heap.cc
Line 4310, Patchset 3 (Latest):
void Heap::EagerlyFreeExternalMemoryAndWasmCode(Omer Katz . unresolved
Does it still make sense to have array buffer sweeping and liftoff code flushing in the same method? I think these are 2 unrelated operations and now they even having differing conditions. I think it makes sense to split this to two separate methods.
Related details
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dec 9, 2025, 11:09:13 AM12/9/25
to V8 LUCI CQ, Manos Koukoutos, Omer Katz, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Omer Katz
Dominik Inführ added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File src/heap/heap.cc
Does it still make sense to have array buffer sweeping and liftoff code flushing in the same method? I think these are 2 unrelated operations and now they even having differing conditions. I think it makes sense to split this to two separate methods.
Attention is currently required from:
- Omer Katz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Manos Koukoutos (Gerrit)
Dec 9, 2025, 11:48:23 AM12/9/25
to Dominik Inführ, V8 LUCI CQ, Omer Katz, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ and Omer Katz
Manos Koukoutos voted and added 1 comment![]( "Open in Gerrit")
Votes added by Manos Koukoutos
| Code-Review | +1 |
Attention is currently required from:
- Dominik Inführ
- Omer Katz
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Dec 9, 2025, 12:10:25 PM12/9/25
to Dominik Inführ, V8 LUCI CQ, Manos Koukoutos, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Omer Katz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
3 comments
Patchset-level comments
Omer Katz . resolved
lgtm
File src/heap/heap.cc
Line 2061, Patchset 5 (Parent):
void CompleteArrayBufferSweeping(Heap* heap) {Omer Katz . unresolved
nit: This method could stay a free-standing method.
Line 4315, Patchset 2:
gc_reason == GarbageCollectionReason::kMemoryPressure;Dominik Inführ . resolved
We could also revisit whether we want to flush on memory pressure GCs. I guess it makes sense but I don't have a strong opinion here. I would keep it as-is for now until it becomes a problem.
Also afaiu we don't do this for JS but maybe we should.
Omer Katz
Acknowledged
Related details
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dec 9, 2025, 4:40:51 PM12/9/25
to Omer Katz, V8 LUCI CQ, Manos Koukoutos, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
Dominik Inführ voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +2 |
2 comments
Patchset-level comments
Dominik Inführ . resolved
Thanks for the reviews!
File src/heap/heap.cc
Line 2061, Patchset 5 (Parent):
void CompleteArrayBufferSweeping(Heap* heap) {Omer Katz . resolved
nit: This method could stay a free-standing method.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Dec 9, 2025, 5:41:04 PM12/9/25
to Dominik Inführ, Omer Katz, Manos Koukoutos, Hannes Payer, mlippau...@chromium.org, v8-re...@googlegroups.com
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[heap] Limit Liftoff code flushing to last resort GCs
CollectAllAvailableGarbage() was previously always flushing all
Liftoff code. This also meant that code flushing was enabled when
taking heap snapshots.
This CL limits flushing to last resort and memory pressure GCs.
Bug: 466122138
Change-Id: Ibc953385f41dbb8f71f4f49cde954254b4750f07
Reviewed-by: Omer Katz <omer...@chromium.org>
Commit-Queue: Dominik Inführ <dinf...@chromium.org>
Reviewed-by: Manos Koukoutos <mano...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#104216}
Files:
- M src/heap/heap.cc
- M src/heap/heap.h
Change size: S
Delta: 2 files changed, 24 insertions(+), 24 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Omer Katz, +1 by Manos Koukoutos
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages