[api] Adding total allocated bytes in HeapStatistics [v8/v8 : main]

0 views
Skip to first unread message

chromeperf@appspot.gserviceaccount.com (Gerrit)

unread,
Sep 30, 2025, 9:41:17 AMSep 30
to Caio Lima, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Caio Lima and Jakob Kummerow

Message from chrom...@appspot.gserviceaccount.com

📍 Job mac-m1_mini_2020-perf/speedometer3 complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15ee5c5a510000

Open in Gerrit

Related details

Attention is currently required from:
  • Caio Lima
  • Jakob Kummerow
Submit Requirements:
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
Gerrit-Change-Number: 6996467
Gerrit-PatchSet: 2
Gerrit-Owner: Caio Lima <caio...@igalia.com>
Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
Gerrit-CC: Hannes Payer <hpa...@chromium.org>
Gerrit-Attention: Caio Lima <caio...@igalia.com>
Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
Gerrit-Comment-Date: Tue, 30 Sep 2025 13:41:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

chromeperf@appspot.gserviceaccount.com (Gerrit)

unread,
Sep 30, 2025, 9:44:42 AMSep 30
to Caio Lima, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Caio Lima and Jakob Kummerow

Message from chrom...@appspot.gserviceaccount.com

📍 Job mac-m1_mini_2020-perf/jetstream2 complete.

See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11975d08510000

Gerrit-Comment-Date: Tue, 30 Sep 2025 13:44:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

Darius Mercadier (Gerrit)

unread,
Oct 1, 2025, 7:25:53 AMOct 1
to Caio Lima, chrom...@appspot.gserviceaccount.com, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Caio Lima and Jakob Kummerow

Darius Mercadier added 1 comment

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Darius Mercadier . resolved

I ran some pinpoints for you, cf links in the change log of the CL.

FYI, the quicksort-wasm regression in the JetStream2 pinpoint is very likely just noise; I wouldn't worry too much about it.

Open in Gerrit

Related details

Attention is currently required from:
  • Caio Lima
  • Jakob Kummerow
Submit Requirements:
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: v8/v8
Gerrit-Branch: main
Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
Gerrit-Change-Number: 6996467
Gerrit-PatchSet: 2
Gerrit-Owner: Caio Lima <caio...@igalia.com>
Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
Gerrit-CC: Hannes Payer <hpa...@chromium.org>
Gerrit-Attention: Caio Lima <caio...@igalia.com>
Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
Gerrit-Comment-Date: Wed, 01 Oct 2025 11:25:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

Caio Lima (Gerrit)

unread,
Oct 1, 2025, 8:16:22 AMOct 1
to Darius Mercadier, chrom...@appspot.gserviceaccount.com, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
Attention needed from Darius Mercadier and Jakob Kummerow

Caio Lima added 1 comment

Patchset-level comments
Darius Mercadier . unresolved

I ran some pinpoints for you, cf links in the change log of the CL.

FYI, the quicksort-wasm regression in the JetStream2 pinpoint is very likely just noise; I wouldn't worry too much about it.

Caio Lima

Hi Darius, thank you very much for that. When I open either https://pinpoint-dot-chromeperf.appspot.com/job/11975d08510000 and https://pinpoint-dot-chromeperf.appspot.com/job/15ee5c5a510000 I keep getting 403 and can't see the reports. I'm wondering if I'm missing any permission to see them, given this status code.

Open in Gerrit

Related details

Attention is currently required from:
  • Darius Mercadier
  • Jakob Kummerow
Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 2
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Wed, 01 Oct 2025 12:16:18 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Darius Mercadier <dmerc...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Darius Mercadier (Gerrit)

    unread,
    Oct 1, 2025, 9:04:57 AMOct 1
    to Caio Lima, chrom...@appspot.gserviceaccount.com, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima and Jakob Kummerow

    Darius Mercadier added 1 comment

    Patchset-level comments
    Darius Mercadier . unresolved

    I ran some pinpoints for you, cf links in the change log of the CL.

    FYI, the quicksort-wasm regression in the JetStream2 pinpoint is very likely just noise; I wouldn't worry too much about it.

    Caio Lima

    Hi Darius, thank you very much for that. When I open either https://pinpoint-dot-chromeperf.appspot.com/job/11975d08510000 and https://pinpoint-dot-chromeperf.appspot.com/job/15ee5c5a510000 I keep getting 403 and can't see the reports. I'm wondering if I'm missing any permission to see them, given this status code.

    Darius Mercadier

    Mmh, that's annoying.

    I'll give you the TL;DR: seems perfectly neutral on both Speedometer3 and Jestream2. Maybe a slight regression on bomb-workers.First, but I wouldn't be surprised if this is just noise.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Caio Lima
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 2
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Wed, 01 Oct 2025 13:04:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Darius Mercadier <dmerc...@chromium.org>
    Comment-In-Reply-To: Caio Lima <caio...@igalia.com>
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 2, 2025, 1:05:45 PMOct 2
    to Dominik Inführ, Andy Wingo, Darius Mercadier, chrom...@appspot.gserviceaccount.com, Jakob Kummerow, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Dominik Inführ and Jakob Kummerow

    Caio Lima added 1 comment

    Patchset-level comments
    File-level comment, Patchset 6 (Latest):
    Caio Lima . resolved

    Hey Dominik, since a lot of code that I was reading to understand how to implement this counter you were as the last author, I'm picking you as possible reviewer here. Sorry if you are not the best reviewer here.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 6
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Thu, 02 Oct 2025 17:05:40 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    open
    diffy

    Andy Wingo (Gerrit)

    unread,
    Oct 8, 2025, 9:56:28 AMOct 8
    to Caio Lima, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima, Dominik Inführ and Jakob Kummerow

    Andy Wingo added 1 comment

    File src/heap/large-spaces.cc
    Line 152, Patchset 9 (Latest): if (origin != AllocationOrigin::kGC) {
    Andy Wingo . unresolved

    Should this be folded into the previous conditional? If we're not on the main thread, surely this is not an allocation that could go towards the counters. Also, what kinds of allocations go to SHARED_LO_SPACE?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Caio Lima
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Wed, 08 Oct 2025 13:56:13 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    open
    diffy

    Jakob Kummerow (Gerrit)

    unread,
    Oct 8, 2025, 10:03:12 AMOct 8
    to Caio Lima, Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima and Dominik Inführ

    Jakob Kummerow added 1 comment

    File src/heap/large-spaces.cc
    Line 152, Patchset 9 (Latest): if (origin != AllocationOrigin::kGC) {
    Andy Wingo . unresolved

    Should this be folded into the previous conditional? If we're not on the main thread, surely this is not an allocation that could go towards the counters. Also, what kinds of allocations go to SHARED_LO_SPACE?

    Jakob Kummerow

    what kinds of allocations go to SHARED_LO_SPACE?

    Related details

    Attention is currently required from:
    • Caio Lima
    • Dominik Inführ
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Wed, 08 Oct 2025 14:02:56 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Andy Wingo <wi...@igalia.com>
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 8, 2025, 10:25:11 AMOct 8
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Dominik Inführ and Jakob Kummerow

    Caio Lima added 1 comment

    File src/heap/large-spaces.cc
    Line 152, Patchset 9 (Latest): if (origin != AllocationOrigin::kGC) {
    Andy Wingo . unresolved

    Should this be folded into the previous conditional? If we're not on the main thread, surely this is not an allocation that could go towards the counters. Also, what kinds of allocations go to SHARED_LO_SPACE?

    Jakob Kummerow

    what kinds of allocations go to SHARED_LO_SPACE?

    Objects that are both shared (per https://github.com/tc39/proposal-structs or https://github.com/WebAssembly/shared-everything-threads) and too large for non-LO spaces.

    Caio Lima

    If we fold this into the condition above, we will not count the allocations of those shared objects that Jakob mentioned. I can see that a JSSharedArray always allocate with `AllocationType::kSharedOld` to allocate its storage in `Factory::NewJSSharedArray`.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Wed, 08 Oct 2025 14:25:02 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Andy Wingo <wi...@igalia.com>
    Comment-In-Reply-To: Jakob Kummerow <jkum...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 8, 2025, 10:29:26 AMOct 8
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Caio Lima and Jakob Kummerow

    Dominik Inführ added 6 comments

    File src/heap/heap-allocator.cc
    Line 110, Patchset 8: }
    Dominik Inführ . unresolved

    If allocation was successful, we could just run `total_allocated_bytes_ += size_in_bytes` here.

    File src/heap/large-spaces.cc
    Line 152, Patchset 8: if (origin != AllocationOrigin::kGC) {
    Dominik Inführ . unresolved

    We shouldn't really have large object allocations in the GC? That's probably the reason we don't have the AllocationOrigin here.

    Line 153, Patchset 8: heap()->isolate()->AddTotalAllocatedBytes(object_size);
    Dominik Inführ . unresolved

    Here we probably should report back to HeapAllocator instead with `local_heap->allocator()->AddTotalAllocatedBytes(..)`. That would make this operation thread-local and the HeapAllocator would know about all allocations.

    Although thinking about this more: What about adding this in HeapAllocator::AllocateRawLargeInternal? That's the bottleneck for all large allocations, so you would only need to do it once and not once per space like currently.

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    File src/heap/spaces.cc
    Line 66, Patchset 8:uint64_t Space::GetTotalAllocatedBytesInLAA() const {
    Dominik Inführ . unresolved

    I think this method should go to HeapAllocator instead. The idea here is that the Space doesn't know about LABs anymore.

    Ideally we also wouldn't report a value per Space because afaics we only care about the total number in the end, right? With `HeapAllocator::GetTotalAllocatedBytes()`, supporting background threads would be quite simple as well: `for (LocalHeap* local_heap: xx) total += local_heap->allocator()->GetTotalAllocatedBytes()` (although we would need a safepoint).

    Line 104, Patchset 8: total_bytes += space_allocator->top() - space_allocator->start();
    Dominik Inführ . unresolved

    I believe as part of GetHeapStatistics/GetHeapSpaceStatistics we already free the main-thread LABs, see [0]. So in theory at least this should be always 0 here and we shouldn't need to handle this here.

    0: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=10374;drc=f8fa8e7d227a3100de378181627a69818a7f881c

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Caio Lima
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 9
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Wed, 08 Oct 2025 14:29:11 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 8, 2025, 10:31:47 AMOct 8
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Caio Lima and Jakob Kummerow

    Dominik Inführ added 1 comment

    Patchset-level comments
    File-level comment, Patchset 9 (Latest):
    Dominik Inführ . resolved

    Sorry, apparently I reviewed your changes but didn't submit my comments until just now. Please look over them but some of them might be either outdated or not show up at the right code location anymore.

    Gerrit-Comment-Date: Wed, 08 Oct 2025 14:31:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 9, 2025, 1:55:05 PM (14 days ago) Oct 9
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 7 comments

    Patchset-level comments
    File-level comment, Patchset 2:
    Darius Mercadier . resolved

    I ran some pinpoints for you, cf links in the change log of the CL.

    FYI, the quicksort-wasm regression in the JetStream2 pinpoint is very likely just noise; I wouldn't worry too much about it.

    Caio Lima

    Hi Darius, thank you very much for that. When I open either https://pinpoint-dot-chromeperf.appspot.com/job/11975d08510000 and https://pinpoint-dot-chromeperf.appspot.com/job/15ee5c5a510000 I keep getting 403 and can't see the reports. I'm wondering if I'm missing any permission to see them, given this status code.

    Darius Mercadier

    Mmh, that's annoying.

    I'll give you the TL;DR: seems perfectly neutral on both Speedometer3 and Jestream2. Maybe a slight regression on bomb-workers.First, but I wouldn't be surprised if this is just noise.

    Caio Lima

    Acknowledged

    File src/heap/heap-allocator.cc
    Dominik Inführ . unresolved

    If allocation was successful, we could just run `total_allocated_bytes_ += size_in_bytes` here.

    Caio Lima

    I've changed the patch to do count allocation here.

    File src/heap/large-spaces.cc
    Line 152, Patchset 8: if (origin != AllocationOrigin::kGC) {
    Dominik Inführ . unresolved

    We shouldn't really have large object allocations in the GC? That's probably the reason we don't have the AllocationOrigin here.

    Caio Lima

    Sorry I missed this. I reverted those changes and now I'm counting on `HeapAllocator::AllocateRawLargeInternal`. Thanks a lot for pointing it out.

    Line 153, Patchset 8: heap()->isolate()->AddTotalAllocatedBytes(object_size);
    Dominik Inführ . resolved

    Here we probably should report back to HeapAllocator instead with `local_heap->allocator()->AddTotalAllocatedBytes(..)`. That would make this operation thread-local and the HeapAllocator would know about all allocations.

    Although thinking about this more: What about adding this in HeapAllocator::AllocateRawLargeInternal? That's the bottleneck for all large allocations, so you would only need to do it once and not once per space like currently.

    Caio Lima

    Acknowledged

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    File src/heap/spaces.cc
    Line 66, Patchset 8:uint64_t Space::GetTotalAllocatedBytesInLAA() const {
    Dominik Inführ . unresolved

    I think this method should go to HeapAllocator instead. The idea here is that the Space doesn't know about LABs anymore.

    Ideally we also wouldn't report a value per Space because afaics we only care about the total number in the end, right? With `HeapAllocator::GetTotalAllocatedBytes()`, supporting background threads would be quite simple as well: `for (LocalHeap* local_heap: xx) total += local_heap->allocator()->GetTotalAllocatedBytes()` (although we would need a safepoint).

    Caio Lima

    I moved most of the logic to `HeapAllocator` instead, together with the counter. Looks like it makes more sense things be there instead of in the Isolate.

    Additionally I've changed how `GetTotalAllocatedBytes` gets calculated and I'm now iterating over LocalHeaps with safepoint logic inside a `Heap::GettotalAllocatedBytes()`. Let me know if this is what you had in mind.

    Line 104, Patchset 8: total_bytes += space_allocator->top() - space_allocator->start();
    Dominik Inführ . unresolved

    I believe as part of GetHeapStatistics/GetHeapSpaceStatistics we already free the main-thread LABs, see [0]. So in theory at least this should be always 0 here and we shouldn't need to handle this here.

    0: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=10374;drc=f8fa8e7d227a3100de378181627a69818a7f881c

    Caio Lima

    That's true. In my current implementation I also want to take into account allocations that happens on non-GC workers (e.g compiler threads) and I'm wondering about their LABs. They seem to get their own LocalHeap and IIUC their LABs aren't freed when GetHeapStatistics gets called. Does it still make sense to keep such logic of inspecting LABs there?

    Additionally, after looking deeper into the code, I think LocalHeaps aren't created for GC workers, so when I iterate over `Heap::safepoint()->IterateLocalHeaps`, I won't get any allocation for GC. Is this correct?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Darius Mercadier
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 10
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Thu, 09 Oct 2025 17:54:55 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Darius Mercadier <dmerc...@chromium.org>
    Comment-In-Reply-To: Caio Lima <caio...@igalia.com>
    Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 9, 2025, 3:13:23 PM (14 days ago) Oct 9
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Caio Lima, Darius Mercadier and Jakob Kummerow

    Dominik Inführ added 6 comments

    File src/heap/heap-allocator.h
    Line 116, Patchset 12 (Latest): void AddTotalAllocatedBytes(size_t size) { total_allocated_bytes_ += size; }
    Dominik Inführ . unresolved

    I suppose we don't need to expose this anymore.

    File src/heap/heap-allocator.cc
    Line 110, Patchset 8: }
    Dominik Inführ . resolved

    If allocation was successful, we could just run `total_allocated_bytes_ += size_in_bytes` here.

    Caio Lima

    I've changed the patch to do count allocation here.

    Dominik Inführ

    Acknowledged

    Line 337, Patchset 12 (Latest): if (new_space_allocator_) {
    Dominik Inführ . unresolved

    The LABs should be all empty here, you can add DHECKs like `DCHECK(!old_space_allocator->IsLabValid())` or `DHECK_IMPLIES(new_space_allocator_, !new_space_allocator->IsLabValid()` that this indeed holds. Then this method is just `return total_allocated_bytes_` with some additional DCHECKs.

    File src/heap/heap.cc
    Line 7884, Patchset 12 (Latest): SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);
    Dominik Inführ . unresolved

    We currently don't have a safepoint in GetHeapStatistics(). I need to check whether this is okay. I would move the safepoint to GetHeapStatistics() though. Here it is a bit hidden.

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    Dominik Inführ

    AdvanceAllocationObservers() doesn't feel like the right location for this. Why not add this to ResetLab() instead? In there you would simply increment your counter by `end-start`.

    File src/heap/spaces.cc
    Line 104, Patchset 8: total_bytes += space_allocator->top() - space_allocator->start();
    Dominik Inführ . unresolved

    I believe as part of GetHeapStatistics/GetHeapSpaceStatistics we already free the main-thread LABs, see [0]. So in theory at least this should be always 0 here and we shouldn't need to handle this here.

    0: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=10374;drc=f8fa8e7d227a3100de378181627a69818a7f881c

    Caio Lima

    That's true. In my current implementation I also want to take into account allocations that happens on non-GC workers (e.g compiler threads) and I'm wondering about their LABs. They seem to get their own LocalHeap and IIUC their LABs aren't freed when GetHeapStatistics gets called. Does it still make sense to keep such logic of inspecting LABs there?

    Additionally, after looking deeper into the code, I think LocalHeaps aren't created for GC workers, so when I iterate over `Heap::safepoint()->IterateLocalHeaps`, I won't get any allocation for GC. Is this correct?

    Dominik Inführ

    With a safepoint you can just invoke `FreeLinearAllocationAreas()` to free the LABs for background threads as well. Because we don't have a safepoint yet, we only free LABs them on the main thread atm. I don't think it's really necessary to account for `top-start` anywhere, we should just free the LABs.

    In theory I guess we could avoid the safepoint but I would like to avoid that.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Caio Lima
    • Darius Mercadier
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 12
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Thu, 09 Oct 2025 19:13:05 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 10, 2025, 8:33:36 AM (13 days ago) Oct 10
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 3 comments

    Patchset-level comments
    File-level comment, Patchset 12 (Latest):
    Caio Lima . resolved

    Thank you very much for the review so far, Dominik.

    File src/heap/heap.cc
    Line 7884, Patchset 12 (Latest): SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);
    Dominik Inführ . unresolved

    We currently don't have a safepoint in GetHeapStatistics(). I need to check whether this is okay. I would move the safepoint to GetHeapStatistics() though. Here it is a bit hidden.

    Caio Lima

    This is nice, mainly when combined to your comment bellow asking to always free LienarAllocationAreas.

    I'm curious to understand how you'd check if it's ok to place a safepoint in `GetHeapStatistics`. I'd like to understand it and it's likely that I misunderstand how safepoints work. My understanding so far is that when I call `SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);`, it will wait for all workers to report `Safepoint()` and Park their thread until `~SafepointScope` is called. Once all workers for this Isolate are parked, the rest of the code continues and I can be sure that when calling `local_heap->allocator()->GetTotalAllocatedBytes()` I won't get a race. Is it correct?

    Based on that, I thought that moving it to `GetHeapStatistics` should just work, but from your comment, I'm overlooking something else.

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    Dominik Inführ

    AdvanceAllocationObservers() doesn't feel like the right location for this. Why not add this to ResetLab() instead? In there you would simply increment your counter by `end-start`.

    Caio Lima

    The issue that I'm getting here is because by the end of the `AdvanceAllocationObservers` we call `MarkLabInitialized`[1]. The `AllocationInfo` get's its `start` changed to `top`, which makes all calculation after `AdvanceAllocationObservers` miss some bytes. I've got this problem while implementing my patch, since I had some accounting happening after `AdvanceAllocationObservers`. Given that, if I place the logic to count on ResetLab, I'll get 0 in when this path is executed here[2], for example. That's the reason why I initially implemented the `AddTotalAllocatedBytes` in three places before, and always calling before `AdvanceAllocationObservers`. I can do it in `RestLab` if this isn't a blocker, but I'm not sure why we call a `MarkLabStartInitialized` by the end of `AdvanceAllocationObservers`. Do you know why it's called there? TBH, I'm quite in favor to move that from there, because it took me quite a while to debug why my counter was quite low and I was very surprised to see that `AdvanceAllocationObservers` changes `AllocationInfo`'s start.

    [1] -https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=130
    [2]- https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=526

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Darius Mercadier
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 12
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Fri, 10 Oct 2025 12:33:26 +0000
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 10, 2025, 9:48:50 AM (13 days ago) Oct 10
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 1 comment

    File src/heap/heap.cc
    Line 7884, Patchset 12 (Latest): SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);
    Dominik Inführ . unresolved

    We currently don't have a safepoint in GetHeapStatistics(). I need to check whether this is okay. I would move the safepoint to GetHeapStatistics() though. Here it is a bit hidden.

    Caio Lima

    This is nice, mainly when combined to your comment bellow asking to always free LienarAllocationAreas.

    I'm curious to understand how you'd check if it's ok to place a safepoint in `GetHeapStatistics`. I'd like to understand it and it's likely that I misunderstand how safepoints work. My understanding so far is that when I call `SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);`, it will wait for all workers to report `Safepoint()` and Park their thread until `~SafepointScope` is called. Once all workers for this Isolate are parked, the rest of the code continues and I can be sure that when calling `local_heap->allocator()->GetTotalAllocatedBytes()` I won't get a race. Is it correct?

    Based on that, I thought that moving it to `GetHeapStatistics` should just work, but from your comment, I'm overlooking something else.

    Caio Lima

    I think I've found an issue here. From this comment[1], `GetHeapStatistics` not necessarily comes from main thread, and this would make the `SafepointScope` fail `DCHECK(LocalHeap::Current()->is_main_thread());` when it's called out of main thread. It seems a breaking change if it's required that `GetHeapStatistics` needs to be called from Main Thread. I see a couple of directions here:

    1. For our use cases, it's probably fine to only count allocations that happens in main thread. This way, we would keep this new field to `HeapStatistics`. However I feel that this isn't very nice approach, because I think the API extension should not be specialized for those use cases.
    2. We could actually change the API a bit and create a public `Isolate::GetTotalAllocatedBytes` where it's required to be called from main thread. This probably would allow the use Safepoint and the counting should work fine.

    I prefer the second option, because it seems a better way to extend the API supporting allocations from workers and not introducing any break changes in current API.

    Gerrit-Comment-Date: Fri, 10 Oct 2025 13:48:42 +0000
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 13, 2025, 3:38:44 AM (10 days ago) Oct 13
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima, Darius Mercadier and Jakob Kummerow

    Dominik Inführ added 3 comments

    File src/heap/heap.cc
    Line 7884, Patchset 12 (Latest): SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);
    Dominik Inführ . unresolved

    We currently don't have a safepoint in GetHeapStatistics(). I need to check whether this is okay. I would move the safepoint to GetHeapStatistics() though. Here it is a bit hidden.

    Caio Lima

    This is nice, mainly when combined to your comment bellow asking to always free LienarAllocationAreas.

    I'm curious to understand how you'd check if it's ok to place a safepoint in `GetHeapStatistics`. I'd like to understand it and it's likely that I misunderstand how safepoints work. My understanding so far is that when I call `SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);`, it will wait for all workers to report `Safepoint()` and Park their thread until `~SafepointScope` is called. Once all workers for this Isolate are parked, the rest of the code continues and I can be sure that when calling `local_heap->allocator()->GetTotalAllocatedBytes()` I won't get a race. Is it correct?

    Based on that, I thought that moving it to `GetHeapStatistics` should just work, but from your comment, I'm overlooking something else.

    Caio Lima

    I think I've found an issue here. From this comment[1], `GetHeapStatistics` not necessarily comes from main thread, and this would make the `SafepointScope` fail `DCHECK(LocalHeap::Current()->is_main_thread());` when it's called out of main thread. It seems a breaking change if it's required that `GetHeapStatistics` needs to be called from Main Thread. I see a couple of directions here:

    1. For our use cases, it's probably fine to only count allocations that happens in main thread. This way, we would keep this new field to `HeapStatistics`. However I feel that this isn't very nice approach, because I think the API extension should not be specialized for those use cases.
    2. We could actually change the API a bit and create a public `Isolate::GetTotalAllocatedBytes` where it's required to be called from main thread. This probably would allow the use Safepoint and the counting should work fine.

    I prefer the second option, because it seems a better way to extend the API supporting allocations from workers and not introducing any break changes in current API.

    Dominik Inführ

    No, sorry. You are not overlooking anything. I was just wondering whether it is okay to do this because of performance - not because of correctness. At some point iirc we unintentionally had such calls during benchmarks. Safepointing shouldn't be too costly but I still wanted to check.

    I think we already rely on GetHeapStatistics only being used from the main thread (see [0]). So adding a safepoint there shouldn't be an issue from that point-of-view. Having the safepoint in GetHeapStatistics would make this a lot simpler, we could then simply free all LABs (not just main thread) and simply sum up all per-thread counters (no atomics or synchronization required).

    0: https://issues.chromium.org/u/1/issues/345822325

    File src/heap/large-spaces.cc
    Line 152, Patchset 8: if (origin != AllocationOrigin::kGC) {
    Dominik Inführ . resolved

    We shouldn't really have large object allocations in the GC? That's probably the reason we don't have the AllocationOrigin here.

    Caio Lima

    Sorry I missed this. I reverted those changes and now I'm counting on `HeapAllocator::AllocateRawLargeInternal`. Thanks a lot for pointing it out.

    Dominik Inführ

    Acknowledged

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    Dominik Inführ

    AdvanceAllocationObservers() doesn't feel like the right location for this. Why not add this to ResetLab() instead? In there you would simply increment your counter by `end-start`.

    Caio Lima

    The issue that I'm getting here is because by the end of the `AdvanceAllocationObservers` we call `MarkLabInitialized`[1]. The `AllocationInfo` get's its `start` changed to `top`, which makes all calculation after `AdvanceAllocationObservers` miss some bytes. I've got this problem while implementing my patch, since I had some accounting happening after `AdvanceAllocationObservers`. Given that, if I place the logic to count on ResetLab, I'll get 0 in when this path is executed here[2], for example. That's the reason why I initially implemented the `AddTotalAllocatedBytes` in three places before, and always calling before `AdvanceAllocationObservers`. I can do it in `RestLab` if this isn't a blocker, but I'm not sure why we call a `MarkLabStartInitialized` by the end of `AdvanceAllocationObservers`. Do you know why it's called there? TBH, I'm quite in favor to move that from there, because it took me quite a while to debug why my counter was quite low and I was very surprised to see that `AdvanceAllocationObservers` changes `AllocationInfo`'s start.

    [1] -https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=130
    [2]- https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=526

    Dominik Inführ

    AllocationObservers and their accounting is indeed quite tricky. For better or worse it uses the size of [start,top] for accounting. If I could redo this from scratch, it would probably look quite differently today. That's why I was thinking about using ResetLab() for this feature instead. That method should be called every time we set up a LAB. At that point we still have start==top in the MainAllocator's `allocation_info_`. The boundaries are passed to `ResetLab(start, end)`, so we can use them to directly bump the counter there before even filling the LAB.

    E.g. I believe this should already work:

    ```
    void MainAllocator::ResetLab(..) {
    ...
    total_allocated_bytes_ += (end-start);
    allocation_info().Reset(start, end);
    ....
    }
    ```

    I have to admit that this has the disadvantage that we might overapproximate allocations a bit if we don't end up using the whole LAB but in relation to all the other flakiness/background threads/etc. it should still be negligible relative to how much is allocated in general. I would prefer going with this first to land the CL sooner. And if necessary we could refine this as a follow-up.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Caio Lima
    • Darius Mercadier
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 12
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Mon, 13 Oct 2025 07:38:25 +0000
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 13, 2025, 12:01:53 PM (10 days ago) Oct 13
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Andy Wingo, Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 6 comments

    File src/heap/heap-allocator.h
    Line 116, Patchset 12: void AddTotalAllocatedBytes(size_t size) { total_allocated_bytes_ += size; }
    Dominik Inführ . unresolved

    I suppose we don't need to expose this anymore.

    Caio Lima

    I'm not sure if I missed anything here, but since I'm counting from `MainAllocator::ResetLab` and the counter is in `HeapAllocator`, I think I'd still need it here, no?

    File src/heap/heap-allocator.cc
    Line 337, Patchset 12: if (new_space_allocator_) {
    Dominik Inführ . resolved

    The LABs should be all empty here, you can add DHECKs like `DCHECK(!old_space_allocator->IsLabValid())` or `DHECK_IMPLIES(new_space_allocator_, !new_space_allocator->IsLabValid()` that this indeed holds. Then this method is just `return total_allocated_bytes_` with some additional DCHECKs.

    Caio Lima

    Acknowledged

    File src/heap/heap.cc
    Line 7884, Patchset 12: SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);
    Dominik Inführ . resolved

    We currently don't have a safepoint in GetHeapStatistics(). I need to check whether this is okay. I would move the safepoint to GetHeapStatistics() though. Here it is a bit hidden.

    Caio Lima

    This is nice, mainly when combined to your comment bellow asking to always free LienarAllocationAreas.

    I'm curious to understand how you'd check if it's ok to place a safepoint in `GetHeapStatistics`. I'd like to understand it and it's likely that I misunderstand how safepoints work. My understanding so far is that when I call `SafepointScope safe_scope(isolate_, SafepointKind::kIsolate);`, it will wait for all workers to report `Safepoint()` and Park their thread until `~SafepointScope` is called. Once all workers for this Isolate are parked, the rest of the code continues and I can be sure that when calling `local_heap->allocator()->GetTotalAllocatedBytes()` I won't get a race. Is it correct?

    Based on that, I thought that moving it to `GetHeapStatistics` should just work, but from your comment, I'm overlooking something else.

    Caio Lima

    I think I've found an issue here. From this comment[1], `GetHeapStatistics` not necessarily comes from main thread, and this would make the `SafepointScope` fail `DCHECK(LocalHeap::Current()->is_main_thread());` when it's called out of main thread. It seems a breaking change if it's required that `GetHeapStatistics` needs to be called from Main Thread. I see a couple of directions here:

    1. For our use cases, it's probably fine to only count allocations that happens in main thread. This way, we would keep this new field to `HeapStatistics`. However I feel that this isn't very nice approach, because I think the API extension should not be specialized for those use cases.
    2. We could actually change the API a bit and create a public `Isolate::GetTotalAllocatedBytes` where it's required to be called from main thread. This probably would allow the use Safepoint and the counting should work fine.

    I prefer the second option, because it seems a better way to extend the API supporting allocations from workers and not introducing any break changes in current API.

    Dominik Inführ

    No, sorry. You are not overlooking anything. I was just wondering whether it is okay to do this because of performance - not because of correctness. At some point iirc we unintentionally had such calls during benchmarks. Safepointing shouldn't be too costly but I still wanted to check.

    I think we already rely on GetHeapStatistics only being used from the main thread (see [0]). So adding a safepoint there shouldn't be an issue from that point-of-view. Having the safepoint in GetHeapStatistics would make this a lot simpler, we could then simply free all LABs (not just main thread) and simply sum up all per-thread counters (no atomics or synchronization required).

    0: https://issues.chromium.org/u/1/issues/345822325

    Caio Lima

    Acknowledged

    File src/heap/large-spaces.cc
    Line 152, Patchset 9: if (origin != AllocationOrigin::kGC) {
    Andy Wingo . resolved

    Should this be folded into the previous conditional? If we're not on the main thread, surely this is not an allocation that could go towards the counters. Also, what kinds of allocations go to SHARED_LO_SPACE?

    Jakob Kummerow

    what kinds of allocations go to SHARED_LO_SPACE?

    Objects that are both shared (per https://github.com/tc39/proposal-structs or https://github.com/WebAssembly/shared-everything-threads) and too large for non-LO spaces.

    Caio Lima

    If we fold this into the condition above, we will not count the allocations of those shared objects that Jakob mentioned. I can see that a JSSharedArray always allocate with `AllocationType::kSharedOld` to allocate its storage in `Factory::NewJSSharedArray`.

    Caio Lima

    Done

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . unresolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    Dominik Inführ

    AdvanceAllocationObservers() doesn't feel like the right location for this. Why not add this to ResetLab() instead? In there you would simply increment your counter by `end-start`.

    Caio Lima

    The issue that I'm getting here is because by the end of the `AdvanceAllocationObservers` we call `MarkLabInitialized`[1]. The `AllocationInfo` get's its `start` changed to `top`, which makes all calculation after `AdvanceAllocationObservers` miss some bytes. I've got this problem while implementing my patch, since I had some accounting happening after `AdvanceAllocationObservers`. Given that, if I place the logic to count on ResetLab, I'll get 0 in when this path is executed here[2], for example. That's the reason why I initially implemented the `AddTotalAllocatedBytes` in three places before, and always calling before `AdvanceAllocationObservers`. I can do it in `RestLab` if this isn't a blocker, but I'm not sure why we call a `MarkLabStartInitialized` by the end of `AdvanceAllocationObservers`. Do you know why it's called there? TBH, I'm quite in favor to move that from there, because it took me quite a while to debug why my counter was quite low and I was very surprised to see that `AdvanceAllocationObservers` changes `AllocationInfo`'s start.

    [1] -https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=130
    [2]- https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=526

    Dominik Inführ

    AllocationObservers and their accounting is indeed quite tricky. For better or worse it uses the size of [start,top] for accounting. If I could redo this from scratch, it would probably look quite differently today. That's why I was thinking about using ResetLab() for this feature instead. That method should be called every time we set up a LAB. At that point we still have start==top in the MainAllocator's `allocation_info_`. The boundaries are passed to `ResetLab(start, end)`, so we can use them to directly bump the counter there before even filling the LAB.

    E.g. I believe this should already work:

    ```
    void MainAllocator::ResetLab(..) {
    ...
    total_allocated_bytes_ += (end-start);
    allocation_info().Reset(start, end);
    ....
    }
    ```

    I have to admit that this has the disadvantage that we might overapproximate allocations a bit if we don't end up using the whole LAB but in relation to all the other flakiness/background threads/etc. it should still be negligible relative to how much is allocated in general. I would prefer going with this first to land the CL sooner. And if necessary we could refine this as a follow-up.

    Caio Lima

    I've implemented and tested this version. From the toy example I'm using here, the overestimation is about ~1%, which seems ok.

    File src/heap/spaces.cc
    Line 104, Patchset 8: total_bytes += space_allocator->top() - space_allocator->start();
    Dominik Inführ . resolved

    I believe as part of GetHeapStatistics/GetHeapSpaceStatistics we already free the main-thread LABs, see [0]. So in theory at least this should be always 0 here and we shouldn't need to handle this here.

    0: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=10374;drc=f8fa8e7d227a3100de378181627a69818a7f881c

    Caio Lima

    That's true. In my current implementation I also want to take into account allocations that happens on non-GC workers (e.g compiler threads) and I'm wondering about their LABs. They seem to get their own LocalHeap and IIUC their LABs aren't freed when GetHeapStatistics gets called. Does it still make sense to keep such logic of inspecting LABs there?

    Additionally, after looking deeper into the code, I think LocalHeaps aren't created for GC workers, so when I iterate over `Heap::safepoint()->IterateLocalHeaps`, I won't get any allocation for GC. Is this correct?

    Dominik Inführ

    With a safepoint you can just invoke `FreeLinearAllocationAreas()` to free the LABs for background threads as well. Because we don't have a safepoint yet, we only free LABs them on the main thread atm. I don't think it's really necessary to account for `top-start` anywhere, we should just free the LABs.

    In theory I guess we could avoid the safepoint but I would like to avoid that.

    Caio Lima

    Acknowledged

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Wingo
    • Darius Mercadier
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 14
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Andy Wingo <wi...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Mon, 13 Oct 2025 16:01:40 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Caio Lima <caio...@igalia.com>
    Comment-In-Reply-To: Andy Wingo <wi...@igalia.com>
    Comment-In-Reply-To: Jakob Kummerow <jkum...@chromium.org>
    Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 14, 2025, 2:57:07 AM (9 days ago) Oct 14
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima, Darius Mercadier and Jakob Kummerow

    Dominik Inführ added 1 comment

    File src/heap/heap-allocator.h
    Line 116, Patchset 12: void AddTotalAllocatedBytes(size_t size) { total_allocated_bytes_ += size; }
    Dominik Inführ . resolved

    I suppose we don't need to expose this anymore.

    Caio Lima

    I'm not sure if I missed anything here, but since I'm counting from `MainAllocator::ResetLab` and the counter is in `HeapAllocator`, I think I'd still need it here, no?

    Dominik Inführ

    Right, sorry...

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Caio Lima
    • Darius Mercadier
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 15
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Caio Lima <caio...@igalia.com>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Comment-Date: Tue, 14 Oct 2025 06:56:49 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Caio Lima <caio...@igalia.com>
    Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Dominik Inführ (Gerrit)

    unread,
    Oct 16, 2025, 7:46:28 AM (7 days ago) Oct 16
    to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Caio Lima, Darius Mercadier and Jakob Kummerow

    Dominik Inführ added 5 comments

    Patchset-level comments
    File-level comment, Patchset 15 (Latest):
    Dominik Inführ . resolved

    In general this CL already LGTM but my colleagues prefer the non-safepoint variant. Added a few comments.

    File src/api/api.cc
    Line 10168, Patchset 15 (Latest): i::SafepointKind safepoint_kind = i_isolate->is_shared_space_isolate()
    Dominik Inführ . unresolved

    Sorry to have misled but my colleagues would prefer to keep the safepoint out of this method. It should not be too hard to change this though.

    File src/heap/heap-allocator.h
    Line 116, Patchset 15 (Latest): void AddTotalAllocatedBytes(size_t size) { total_allocated_bytes_ += size; }
    Dominik Inführ . unresolved

    This one now needs to increment the counter in heap. This should be something like `heap()->total_allocated_bytes.fetch_add(size, std::memory_order_relaxed)`.

    File src/heap/heap.cc
    Line 7885, Patchset 15 (Latest): safepoint()->IterateLocalHeaps([&](LocalHeap* local_heap) {
    Dominik Inführ . unresolved

    Let's add an atomic Heap::total_allocated_bytes_ field and return that field relaxed here.

    File src/heap/main-allocator.cc
    Line 526, Patchset 8: if (!allocator_->in_gc()) {
    Dominik Inführ . resolved

    Can we do that in MainAllocator directly? E.g. in ResetLab(). That way we wouldn't need to do this 3x but only once. The "AllocatorPolicy" stuff should really be only for logic in the MainAllocator that's space-dependent.

    Caio Lima

    I moved this to `MainAllocator::AdvanceAllocationObservers`. I placed there because it calls `MainAllocator::MarkLabStartInitialized` and it resets AllocationInfo's start to top. So when I do `allocator_->top() - allocator_->start()` inside `ResetLab` it quite often results in 0.

    Dominik Inführ

    AdvanceAllocationObservers() doesn't feel like the right location for this. Why not add this to ResetLab() instead? In there you would simply increment your counter by `end-start`.

    Caio Lima

    The issue that I'm getting here is because by the end of the `AdvanceAllocationObservers` we call `MarkLabInitialized`[1]. The `AllocationInfo` get's its `start` changed to `top`, which makes all calculation after `AdvanceAllocationObservers` miss some bytes. I've got this problem while implementing my patch, since I had some accounting happening after `AdvanceAllocationObservers`. Given that, if I place the logic to count on ResetLab, I'll get 0 in when this path is executed here[2], for example. That's the reason why I initially implemented the `AddTotalAllocatedBytes` in three places before, and always calling before `AdvanceAllocationObservers`. I can do it in `RestLab` if this isn't a blocker, but I'm not sure why we call a `MarkLabStartInitialized` by the end of `AdvanceAllocationObservers`. Do you know why it's called there? TBH, I'm quite in favor to move that from there, because it took me quite a while to debug why my counter was quite low and I was very surprised to see that `AdvanceAllocationObservers` changes `AllocationInfo`'s start.

    [1] -https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=130
    [2]- https://source.chromium.org/chromium/chromium/src/+/main:v8/src/heap/main-allocator.cc;l=526

    Dominik Inführ

    AllocationObservers and their accounting is indeed quite tricky. For better or worse it uses the size of [start,top] for accounting. If I could redo this from scratch, it would probably look quite differently today. That's why I was thinking about using ResetLab() for this feature instead. That method should be called every time we set up a LAB. At that point we still have start==top in the MainAllocator's `allocation_info_`. The boundaries are passed to `ResetLab(start, end)`, so we can use them to directly bump the counter there before even filling the LAB.

    E.g. I believe this should already work:

    ```
    void MainAllocator::ResetLab(..) {
    ...
    total_allocated_bytes_ += (end-start);
    allocation_info().Reset(start, end);
    ....
    }
    ```

    I have to admit that this has the disadvantage that we might overapproximate allocations a bit if we don't end up using the whole LAB but in relation to all the other flakiness/background threads/etc. it should still be negligible relative to how much is allocated in general. I would prefer going with this first to land the CL sooner. And if necessary we could refine this as a follow-up.

    Caio Lima

    I've implemented and tested this version. From the toy example I'm using here, the overestimation is about ~1%, which seems ok.

    Dominik Inführ

    Great!

    Gerrit-Comment-Date: Thu, 16 Oct 2025 11:46:21 +0000
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 16, 2025, 2:11:13 PM (7 days ago) Oct 16
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 2 comments

    File src/api/api.cc
    Line 10168, Patchset 15: i::SafepointKind safepoint_kind = i_isolate->is_shared_space_isolate()
    Dominik Inführ . resolved

    Sorry to have misled but my colleagues would prefer to keep the safepoint out of this method. It should not be too hard to change this though.

    Caio Lima

    Acknowledged

    File src/heap/heap.cc
    Line 7885, Patchset 15: safepoint()->IterateLocalHeaps([&](LocalHeap* local_heap) {
    Dominik Inführ . resolved

    Let's add an atomic Heap::total_allocated_bytes_ field and return that field relaxed here.

    Caio Lima

    Acknowledged

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Darius Mercadier
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: v8/v8
    Gerrit-Branch: main
    Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
    Gerrit-Change-Number: 6996467
    Gerrit-PatchSet: 17
    Gerrit-Owner: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
    Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
    Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-CC: Hannes Payer <hpa...@chromium.org>
    Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
    Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
    Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
    Gerrit-Comment-Date: Thu, 16 Oct 2025 18:11:09 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
    unsatisfied_requirement
    open
    diffy

    Caio Lima (Gerrit)

    unread,
    Oct 16, 2025, 2:11:34 PM (7 days ago) Oct 16
    to Andy Wingo, Jakob Kummerow, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
    Attention needed from Darius Mercadier, Dominik Inführ and Jakob Kummerow

    Caio Lima added 2 comments

    File src/heap/heap-allocator.h
    Line 116, Patchset 15: void AddTotalAllocatedBytes(size_t size) { total_allocated_bytes_ += size; }
    Dominik Inführ . resolved

    This one now needs to increment the counter in heap. This should be something like `heap()->total_allocated_bytes.fetch_add(size, std::memory_order_relaxed)`.

    Caio Lima

    Acknowledged

    File src/heap/spaces.cc
    Line 66, Patchset 8:uint64_t Space::GetTotalAllocatedBytesInLAA() const {
    Dominik Inführ . resolved

    I think this method should go to HeapAllocator instead. The idea here is that the Space doesn't know about LABs anymore.

    Ideally we also wouldn't report a value per Space because afaics we only care about the total number in the end, right? With `HeapAllocator::GetTotalAllocatedBytes()`, supporting background threads would be quite simple as well: `for (LocalHeap* local_heap: xx) total += local_heap->allocator()->GetTotalAllocatedBytes()` (although we would need a safepoint).

    Caio Lima

    I moved most of the logic to `HeapAllocator` instead, together with the counter. Looks like it makes more sense things be there instead of in the Isolate.

    Additionally I've changed how `GetTotalAllocatedBytes` gets calculated and I'm now iterating over LocalHeaps with safepoint logic inside a `Heap::GettotalAllocatedBytes()`. Let me know if this is what you had in mind.

    Caio Lima

    Acknowledged

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Darius Mercadier
    • Dominik Inführ
    • Jakob Kummerow
    Submit Requirements:
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: v8/v8
      Gerrit-Branch: main
      Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
      Gerrit-Change-Number: 6996467
      Gerrit-PatchSet: 17
      Gerrit-Owner: Caio Lima <caio...@igalia.com>
      Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
      Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
      Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
      Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
      Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
      Gerrit-CC: Hannes Payer <hpa...@chromium.org>
      Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
      Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
      Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
      Gerrit-Comment-Date: Thu, 16 Oct 2025 18:11:30 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: No
      unsatisfied_requirement
      open
      diffy

      Dominik Inführ (Gerrit)

      unread,
      Oct 20, 2025, 7:44:08 AM (3 days ago) Oct 20
      to Caio Lima, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
      Attention needed from Caio Lima, Darius Mercadier and Jakob Kummerow

      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 17 (Latest):
      Dominik Inführ . resolved

      Thanks, LGTM

      File src/heap/main-allocator.cc
      Line 302, Patchset 17 (Latest): // concurrent optimizations, this seems tolerable.
      Dominik Inführ . unresolved

      Nit: "concurrent optimization" (compilation to generated code) doesn't allocate much, so maybe not the best example. Let's just say that the leftover unused part of the LAB is quite small compared to the LAB itself.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Caio Lima
      • Darius Mercadier
      • Jakob Kummerow
      Submit Requirements:
        • requirement is not satisfiedCode-Owners
        • requirement is not satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement is not satisfiedReview-Enforcement
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: v8/v8
        Gerrit-Branch: main
        Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
        Gerrit-Change-Number: 6996467
        Gerrit-PatchSet: 17
        Gerrit-Owner: Caio Lima <caio...@igalia.com>
        Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
        Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
        Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
        Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
        Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
        Gerrit-CC: Hannes Payer <hpa...@chromium.org>
        Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
        Gerrit-Attention: Caio Lima <caio...@igalia.com>
        Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
        Gerrit-Comment-Date: Mon, 20 Oct 2025 11:44:03 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        unsatisfied_requirement
        open
        diffy

        Caio Lima (Gerrit)

        unread,
        Oct 20, 2025, 8:41:36 AM (3 days ago) Oct 20
        to Dominik Inführ, Andy Wingo, Jakob Kummerow, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
        Attention needed from Darius Mercadier, Dominik Inführ and Jakob Kummerow

        Caio Lima added 2 comments

        Patchset-level comments
        File-level comment, Patchset 18 (Latest):
        Caio Lima . resolved

        Thanks a lot for the review.

        Could someone trigger pinpoints to get numbers from JetStream and Speedometer? From my local tests, there's no regression on benchmarks, but it would be good to get those numbers from bots as well.

        Also, looks like a need another +1 from reviewers, but I'm not sure who can I ask for review here. Do you have any pointer?

        File src/heap/main-allocator.cc
        Line 302, Patchset 17: // concurrent optimizations, this seems tolerable.
        Dominik Inführ . resolved

        Nit: "concurrent optimization" (compilation to generated code) doesn't allocate much, so maybe not the best example. Let's just say that the leftover unused part of the LAB is quite small compared to the LAB itself.

        Caio Lima

        What I had in mind here is an optimization like allocation sink. This can also make the total allocated bytes fluctuate a bit. But I've changed the comment since it seems better suited.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Darius Mercadier
        • Dominik Inführ
        • Jakob Kummerow
        Submit Requirements:
          • requirement is not satisfiedCode-Owners
          • requirement is not satisfiedCode-Review
          • requirement is not satisfiedReview-Enforcement
          Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
          Gerrit-MessageType: comment
          Gerrit-Project: v8/v8
          Gerrit-Branch: main
          Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
          Gerrit-Change-Number: 6996467
          Gerrit-PatchSet: 18
          Gerrit-Owner: Caio Lima <caio...@igalia.com>
          Gerrit-Reviewer: Andy Wingo <wi...@igalia.com>
          Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
          Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
          Gerrit-Reviewer: Jakob Kummerow <jkum...@chromium.org>
          Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
          Gerrit-CC: Hannes Payer <hpa...@chromium.org>
          Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
          Gerrit-Attention: Jakob Kummerow <jkum...@chromium.org>
          Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
          Gerrit-Comment-Date: Mon, 20 Oct 2025 12:41:32 +0000
          Gerrit-HasComments: Yes
          Gerrit-Has-Labels: No
          Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
          unsatisfied_requirement
          open
          diffy

          Dominik Inführ (Gerrit)

          unread,
          Oct 21, 2025, 3:12:43 AM (2 days ago) Oct 21
          to Caio Lima, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
          Attention needed from Caio Lima, Darius Mercadier and Michael Lippautz

          Dominik Inführ added 2 comments

          Patchset-level comments
          Dominik Inführ . resolved

          @mlippautz: Can you ptal as well? We need an additional (api) reviewer.

          Commit Message
          Line 15, Patchset 17:- Total allocation counter is stored in each HeapAllocator, to make the counter thread local
          Dominik Inführ . unresolved

          Nit: Please also add some line breaks to stay within bounds.

          Open in Gerrit

          Related details

          Attention is currently required from:
          • Caio Lima
          • Darius Mercadier
          • Michael Lippautz
          Submit Requirements:
            • requirement is not satisfiedCode-Owners
            • requirement is not satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement is not satisfiedReview-Enforcement
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: v8/v8
            Gerrit-Branch: main
            Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
            Gerrit-Change-Number: 6996467
            Gerrit-PatchSet: 18
            Gerrit-Owner: Caio Lima <caio...@igalia.com>
            Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
            Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
            Gerrit-CC: Hannes Payer <hpa...@chromium.org>
            Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
            Gerrit-Attention: Caio Lima <caio...@igalia.com>
            Gerrit-Attention: Michael Lippautz <mlip...@chromium.org>
            Gerrit-Comment-Date: Tue, 21 Oct 2025 07:12:37 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: No
            unsatisfied_requirement
            open
            diffy

            Dominik Inführ (Gerrit)

            unread,
            Oct 21, 2025, 3:59:31 AM (2 days ago) Oct 21
            to Caio Lima, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
            Attention needed from Caio Lima, Darius Mercadier and Michael Lippautz

            Dominik Inführ added 1 comment

            Patchset-level comments
            Dominik Inführ . resolved

            I've added another reviewer. Bots results look good as well - within usual fluctuation (either neutral or slightly positive).

            (1) https://pinpoint-dot-chromeperf.appspot.com/job/10226588d10000
            (2) https://pinpoint-dot-chromeperf.appspot.com/job/11b13c6b510000.

            Gerrit-Comment-Date: Tue, 21 Oct 2025 07:59:26 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: No
            unsatisfied_requirement
            open
            diffy

            Michael Lippautz (Gerrit)

            unread,
            Oct 21, 2025, 7:55:59 AM (2 days ago) Oct 21
            to Caio Lima, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
            Attention needed from Caio Lima and Darius Mercadier

            Michael Lippautz voted and added 1 comment

            Votes added by Michael Lippautz

            Code-Review+1

            1 comment

            Patchset-level comments
            Michael Lippautz . resolved

            lgtm

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Caio Lima
            • Darius Mercadier
            Submit Requirements:
            • requirement satisfiedCode-Owners
            • requirement is not satisfiedCode-Review
            • requirement is not satisfiedNo-Unresolved-Comments
            • requirement is not satisfiedReview-Enforcement
            Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
            Gerrit-MessageType: comment
            Gerrit-Project: v8/v8
            Gerrit-Branch: main
            Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
            Gerrit-Change-Number: 6996467
            Gerrit-PatchSet: 18
            Gerrit-Owner: Caio Lima <caio...@igalia.com>
            Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
            Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
            Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
            Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
            Gerrit-CC: Hannes Payer <hpa...@chromium.org>
            Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
            Gerrit-Attention: Caio Lima <caio...@igalia.com>
            Gerrit-Comment-Date: Tue, 21 Oct 2025 11:55:54 +0000
            Gerrit-HasComments: Yes
            Gerrit-Has-Labels: Yes
            satisfied_requirement
            unsatisfied_requirement
            open
            diffy

            Caio Lima (Gerrit)

            unread,
            Oct 21, 2025, 10:02:00 AM (2 days ago) Oct 21
            to Michael Lippautz, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
            Attention needed from Darius Mercadier and Dominik Inführ

            Caio Lima added 2 comments

            Patchset-level comments
            File-level comment, Patchset 19 (Latest):
            Caio Lima . resolved

            Thank you very much for the review

            Commit Message
            Line 15, Patchset 17:- Total allocation counter is stored in each HeapAllocator, to make the counter thread local
            Dominik Inführ . resolved

            Nit: Please also add some line breaks to stay within bounds.

            Caio Lima

            Acknowledged

            Open in Gerrit

            Related details

            Attention is currently required from:
            • Darius Mercadier
            • Dominik Inführ
            Submit Requirements:
              • requirement satisfiedCode-Owners
              • requirement is not satisfiedCode-Review
              • requirement is not satisfiedReview-Enforcement
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: v8/v8
              Gerrit-Branch: main
              Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Gerrit-Change-Number: 6996467
              Gerrit-PatchSet: 19
              Gerrit-Owner: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-CC: Hannes Payer <hpa...@chromium.org>
              Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-Attention: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Comment-Date: Tue, 21 Oct 2025 14:01:56 +0000
              Gerrit-HasComments: Yes
              Gerrit-Has-Labels: No
              Comment-In-Reply-To: Dominik Inführ <dinf...@chromium.org>
              satisfied_requirement
              unsatisfied_requirement
              open
              diffy

              Caio Lima (Gerrit)

              unread,
              Oct 21, 2025, 10:13:36 AM (2 days ago) Oct 21
              to Michael Lippautz, Dominik Inführ, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
              Attention needed from Darius Mercadier and Dominik Inführ

              Caio Lima added 1 comment

              Patchset-level comments
              Caio Lima . resolved

              It looks like I need the r+ from Dominik again, because I changed the comment in one in last Patchset. Also since I'm not a committer, I can't ask for CQ to land it yet. Could you help me on that, please?

              Gerrit-Comment-Date: Tue, 21 Oct 2025 14:13:32 +0000
              Gerrit-HasComments: Yes
              Gerrit-Has-Labels: No
              satisfied_requirement
              unsatisfied_requirement
              open
              diffy

              Dominik Inführ (Gerrit)

              unread,
              Oct 21, 2025, 12:46:29 PM (2 days ago) Oct 21
              to Caio Lima, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
              Attention needed from Caio Lima and Darius Mercadier

              Dominik Inführ voted and added 1 comment

              Votes added by Dominik Inführ

              Code-Review+1

              1 comment

              Patchset-level comments
              Dominik Inführ . resolved

              Still LGTM

              Open in Gerrit

              Related details

              Attention is currently required from:
              • Caio Lima
              • Darius Mercadier
              Submit Requirements:
              • requirement satisfiedCode-Owners
              • requirement satisfiedCode-Review
              • requirement satisfiedReview-Enforcement
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: v8/v8
              Gerrit-Branch: main
              Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Gerrit-Change-Number: 6996467
              Gerrit-PatchSet: 19
              Gerrit-Owner: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-CC: Hannes Payer <hpa...@chromium.org>
              Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-Attention: Caio Lima <caio...@igalia.com>
              Gerrit-Comment-Date: Tue, 21 Oct 2025 16:46:24 +0000
              Gerrit-HasComments: Yes
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              open
              diffy

              Caio Lima (Gerrit)

              unread,
              Oct 21, 2025, 12:52:23 PM (2 days ago) Oct 21
              to Dominik Inführ, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
              Attention needed from Darius Mercadier

              Caio Lima voted Commit-Queue+2

              Commit-Queue+2
              Open in Gerrit

              Related details

              Attention is currently required from:
              • Darius Mercadier
              Submit Requirements:
              • requirement satisfiedCode-Owners
              • requirement satisfiedCode-Review
              • requirement satisfiedReview-Enforcement
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: v8/v8
              Gerrit-Branch: main
              Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Gerrit-Change-Number: 6996467
              Gerrit-PatchSet: 19
              Gerrit-Owner: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-CC: Hannes Payer <hpa...@chromium.org>
              Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-Comment-Date: Tue, 21 Oct 2025 16:52:20 +0000
              Gerrit-HasComments: No
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              open
              diffy

              Dmitry Bezhetskov (Gerrit)

              unread,
              Oct 22, 2025, 9:31:49 AM (yesterday) Oct 22
              to Caio Lima, Dominik Inführ, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
              Attention needed from Caio Lima and Darius Mercadier

              Dmitry Bezhetskov voted Commit-Queue+2

              Commit-Queue+2
              Open in Gerrit

              Related details

              Attention is currently required from:
              • Caio Lima
              • Darius Mercadier
              Submit Requirements:
              • requirement satisfiedCode-Owners
              • requirement satisfiedCode-Review
              • requirement satisfiedReview-Enforcement
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: comment
              Gerrit-Project: v8/v8
              Gerrit-Branch: main
              Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Gerrit-Change-Number: 6996467
              Gerrit-PatchSet: 19
              Gerrit-Owner: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Dmitry Bezhetskov <dima...@gmail.com>
              Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-CC: Hannes Payer <hpa...@chromium.org>
              Gerrit-Attention: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-Attention: Caio Lima <caio...@igalia.com>
              Gerrit-Comment-Date: Wed, 22 Oct 2025 13:31:45 +0000
              Gerrit-HasComments: No
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              open
              diffy

              Dmitry Bezhetskov (Gerrit)

              unread,
              Oct 22, 2025, 9:32:31 AM (yesterday) Oct 22
              to Caio Lima, Dominik Inführ, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, V8 LUCI CQ, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com
              Gerrit-Comment-Date: Wed, 22 Oct 2025 13:32:27 +0000
              Gerrit-HasComments: No
              Gerrit-Has-Labels: Yes
              satisfied_requirement
              open
              diffy

              V8 LUCI CQ (Gerrit)

              unread,
              Oct 22, 2025, 10:27:54 AM (yesterday) Oct 22
              to Caio Lima, Dmitry Bezhetskov, Dominik Inführ, Michael Lippautz, Darius Mercadier, chrom...@appspot.gserviceaccount.com, Hannes Payer, cbruni...@chromium.org, mlippau...@chromium.org, v8-re...@googlegroups.com

              V8 LUCI CQ submitted the change

              Change information

              Commit message:
              [api] Adding total allocated bytes in HeapStatistics

              This change exposes total allocated bytes in v8::HeapStatistics API by
              introducing a new total_allocated_bytes() method that tracks all heap
              allocations since an Isolate creation.

              The implementation adds:
              - uint64_t total_allocated_bytes_ field to HeapStatistics.
              - An atomic total allocation counter is stored in the Heap class.
              - The counter is incremented whenever a RestLab is called. This approach can overestimate the total allocation for cases where the LAB is not fully used, but the leftover compared to the LAB itself is quite small, so it seems tolerable.

              Design doc reference:
              https://docs.google.com/document/d/1O4JPsoaxTQsX_7T5Fz4rsGeHMiM16jUrvDuq9FrtbNM
              Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Reviewed-by: Dominik Inführ <dinf...@chromium.org>
              Reviewed-by: Michael Lippautz <mlip...@chromium.org>
              Commit-Queue: Dmitry Bezhetskov <dima...@gmail.com>
              Cr-Commit-Position: refs/heads/main@{#103296}
              Files:
              • M include/v8-statistics.h
              • M src/api/api.cc
              • M src/heap/heap-allocator.cc
              • M src/heap/heap.cc
              • M src/heap/heap.h
              • M src/heap/main-allocator.cc
              • M test/cctest/test-api.cc
              Change size: M
              Delta: 7 files changed, 198 insertions(+), 8 deletions(-)
              Branch: refs/heads/main
              Submit Requirements:
              • requirement satisfiedCode-Review: +1 by Dominik Inführ, +1 by Michael Lippautz
              Open in Gerrit
              Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
              Gerrit-MessageType: merged
              Gerrit-Project: v8/v8
              Gerrit-Branch: main
              Gerrit-Change-Id: Ic531698aaeb1578f943b7fdd346b9159ffd9b6c9
              Gerrit-Change-Number: 6996467
              Gerrit-PatchSet: 20
              Gerrit-Owner: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Caio Lima <caio...@igalia.com>
              Gerrit-Reviewer: Dmitry Bezhetskov <dima...@gmail.com>
              Gerrit-Reviewer: Dominik Inführ <dinf...@chromium.org>
              Gerrit-Reviewer: Michael Lippautz <mlip...@chromium.org>
              Gerrit-CC: Darius Mercadier <dmerc...@chromium.org>
              Gerrit-CC: Hannes Payer <hpa...@chromium.org>
              open
              diffy
              satisfied_requirement
              Reply all
              Reply to author
              Forward
              0 new messages