deps: V8: cherry-pick 0f024d4e66e0

[IlyasShabi]

Original commit message:

[heap profiler] Add is_live field to AllocationProfile::Sample

When using kSamplingIncludeObjectsCollectedByMajorGC/MinorGC flag, samples for collected objects are retained but callers had no way to distinguish live from dead objects.
Add is_live to expose this information.

Change-Id: I2e930644348ff942caa4b192a127c5baa05bbfef
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7603535
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#105698}

Refs: v8/v8@0f024d4

Motivation and changes

Continues the heap profiler improvements started in #62201 and #62273.

V8's StartSamplingHeapProfiler accepts GC flags that retain samples even after the objects are GC'd. However, there was no way to distinguish whether a sampled allocation was still alive or had already been freed.

With this commit, we can:

• Compute allocated vs freed memory
• Identify functions causing GC pressure due to high allocation and more..

As a follow-up, SerializeHeapProfile will be updated to include per-node freed object size based on is_live field from GetSamples().

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/security-wg
• @nodejs/v8-update

[targos]

Is this ABI-compatible?

[IlyasShabi]

Is this ABI-compatible?

This is ABI compatible since the commit adds a new field bool is_live at the end of the AllocationProfile::Sample struct. Node.js already consumes GetSamples() in src/node_worker.cc

[IlyasShabi]

@Qard @targos could you please check this PR again?

[targos]

My understanding of structs is that adding fields cannot be ABI-compatible because it changes the size of the object. @nodejs/cpp-reviewers

[IlyasShabi]

Yes the struct size does matter for ABI compatibility. But AllocationProfile::Sample is never constructed by Node.js, V8 creates and owns them internally. we only reads fields from the returned samples in src/node_worker.cc so adding a field here is safe.

[joyeecheung]

Our V8 header and all the symbols are exposed, so regardless of whether Node.js uses them internally, user land addons can already rely on them (and I think there are probably a long tail of addons or libraries that do, e.g. #61102, note that we don't use the default platform internally)

[IlyasShabi]

That's clear! I understand now how backporting in a patch release could break addons.
Would it make sense to include this commit in #61898 where ABI changes are already expected maybe?

[IlyasShabi]

Keep this PR open until #61898 is merged, then ask the release team to land it in v26.

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2404-intel-64,v8test=v8test/7138/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-power9le,v8test=v8test/7138/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/7138/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72965/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2404-intel-64,v8test=v8test/7157/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/7157/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-power9le,v8test=v8test/7157/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72977/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2404-intel-64,v8test=v8test/7159/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-power9le,v8test=v8test/7159/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/7159/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2404-intel-64,v8test=v8test/7161/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72989/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-power9le,v8test=v8test/7161/

[nodejs-github-bot]

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/7161/

[BridgeAR]

The CI seems very flaky and the V8 CI is green. Is anyone against landing this as is manually?

[RafaelGSS]

The CI seems very flaky and the V8 CI is green. Is anyone against landing this as is manually?

I looked at CI failures, and most of them seem flaky indeed. I'm just not sure about https://ci.nodejs.org/job/node-test-commit-arm-debug/23124/#showFailuresLink - I'd defer to @joyeecheung or someone else more experienced with this group of tests.

[BridgeAR]

@RafaelGSS those were green in a former run: https://ci.nodejs.org/job/node-test-commit-arm-debug/23142/

[IlyasShabi]

Linux contained was green before https://ci.nodejs.org/job/node-test-commit-linux-containered/56015/

[BridgeAR]

Landed in 8385efc

[joyeecheung]

From the reliability reports it seems the flakes only started to appear in the report from April 28 nodejs/reliability#1524 but not on 27 nodejs/reliability#1523 and it's limited to arm64 containers.

It's likely either a regression in the code or in the build. From the code it's not obvious to me which one can trigger it. @nodejs/build any recent changes to the arm64 containers?

[richardlau]

It's likely either a regression in the code or in the build. From the code it's not obvious to me which one can trigger it. @nodejs/build any recent changes to the arm64 containers?

Not within the last three weeks (azure)/two weeks (osuosl) (according to docker ps output).

[IlyasShabi]

According to the April 28 reliability report nodejs/reliability#1524, the arm64 failures started before this PR’s CI run it was around 72902 / 72903