[heap] Use CollectGarbageWithRetry in HandleExternalMemoryInterrupt [v8/v8 : main]

[Etienne Pierre-Doray (Gerrit)]

Attention needed from Dominik Inführ and Michael Lippautz

Etienne Pierre-Doray added 3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Etienne Pierre-Doray . resolved

PTAnL?

File src/heap/heap-allocator.h

Line 142, Patchset 5: bool RetryCustomAllocate(CustomAllocationFunction allocate,

Dominik Inführ . resolved

Nit: AFAICS not having the default argument here doesn't really seem intrusive. Can you require this argument to be explicit here? That way we need to think about its value. IMHO all the RetryCustomAllocateXXX methods should require this argument.

Etienne Pierre-Doray

Done

File src/heap/heap.h

Line 996, Patchset 5: AllocationSpace space, GarbageCollectionReason gc_reason =

Dominik Inführ . resolved

Nit: Maybe here as well. The other CollectGarbageXXX methods don't make this implicit either.

Etienne Pierre-Doray

Done

Open in Gerrit

Related details

Attention is currently required from:

• Dominik Inführ
• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic23ae62de1ac7fd746b06ce441f4ef65d5167c12

Gerrit-Change-Number: 7606727

Gerrit-PatchSet: 6

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chrom...@appspot.gserviceaccount.com <chrom...@appspot.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>

Gerrit-Comment-Date: Mon, 02 Mar 2026 13:08:49 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>

[Etienne Pierre-Doray (Gerrit)]

Attention needed from Dominik Inführ and Michael Lippautz

Etienne Pierre-Doray added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Etienne Pierre-Doray . resolved

+mlippautz@ for src/api

Open in Gerrit

Related details

Attention is currently required from:

• Dominik Inführ
• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic23ae62de1ac7fd746b06ce441f4ef65d5167c12

Gerrit-Change-Number: 7606727

Gerrit-PatchSet: 6

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chrom...@appspot.gserviceaccount.com <chrom...@appspot.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>

Gerrit-Comment-Date: Mon, 02 Mar 2026 13:09:07 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Dominik Inführ (Gerrit)]

Attention needed from Etienne Pierre-Doray and Michael Lippautz

Dominik Inführ voted and added 2 comments

Votes added by Dominik Inführ

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 6:

Dominik Inführ . resolved

Thanks, LGTM modulo that comment

File src/heap/minor-gc-job.cc

Line 183, Patchset 6: NEW_SPACE, GarbageCollectionReason::kFinalizeMarkingViaTask);

Dominik Inführ . unresolved

I think this gc reason is wrong here - as we bail out for incremental marking the line before. This task tries to trigger a new space GC without a stack just before completely filling new space. Depending on how you see it, kAllocationFailure might be off as well but this is the one we use currently and not sure it's worth adding a new gc reason in this CL for this.

Open in Gerrit

Related details

Attention is currently required from:

• Etienne Pierre-Doray
• Michael Lippautz

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.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic23ae62de1ac7fd746b06ce441f4ef65d5167c12

Gerrit-Change-Number: 7606727

Gerrit-PatchSet: 6

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chrom...@appspot.gserviceaccount.com <chrom...@appspot.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Attention: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Comment-Date: Mon, 02 Mar 2026 13:30:05 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Etienne Pierre-Doray (Gerrit)]

Attention needed from Michael Lippautz

Etienne Pierre-Doray added 1 comment

File src/heap/minor-gc-job.cc

Line 183, Patchset 6: NEW_SPACE, GarbageCollectionReason::kFinalizeMarkingViaTask);

Dominik Inführ . resolved

I think this gc reason is wrong here - as we bail out for incremental marking the line before. This task tries to trigger a new space GC without a stack just before completely filling new space. Depending on how you see it, kAllocationFailure might be off as well but this is the one we use currently and not sure it's worth adding a new gc reason in this CL for this.

Etienne Pierre-Doray

Good point, done.

Open in Gerrit

Related details

Attention is currently required from:

• Michael Lippautz

Submit Requirements:

• Code-Owners
• Code-Review

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: v8/v8

Gerrit-Branch: main

Gerrit-Change-Id: Ic23ae62de1ac7fd746b06ce441f4ef65d5167c12

Gerrit-Change-Number: 7606727

Gerrit-PatchSet: 8

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>

Gerrit-CC: AyeAye <android-bu...@system.gserviceaccount.com>

Gerrit-CC: Hannes Payer <hpa...@chromium.org>

Gerrit-CC: V8 LUCI CQ <v8-s...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chrom...@appspot.gserviceaccount.com <chrom...@appspot.gserviceaccount.com>

Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>

Gerrit-Comment-Date: Mon, 02 Mar 2026 13:32:16 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>