Skip to content

deps: V8: cherry-pick 0f024d4e66e0#62408

Closed
IlyasShabi wants to merge 1 commit intonodejs:mainfrom
IlyasShabi:ishabi/v8-islive-sample
Closed

deps: V8: cherry-pick 0f024d4e66e0#62408
IlyasShabi wants to merge 1 commit intonodejs:mainfrom
IlyasShabi:ishabi/v8-islive-sample

Conversation

@IlyasShabi
Copy link
Copy Markdown
Member

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 <[email protected]>
Commit-Queue: Dominik Inführ <[email protected]>
Reviewed-by: Michael Lippautz <[email protected]>
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
Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Mar 23, 2026
@IlyasShabi IlyasShabi requested a review from Qard March 23, 2026 14:14
@IlyasShabi IlyasShabi marked this pull request as ready for review March 23, 2026 14:15
@IlyasShabi IlyasShabi added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 23, 2026
@targos
Copy link
Copy Markdown
Member

targos commented Mar 24, 2026

Is this ABI-compatible?

@IlyasShabi
Copy link
Copy Markdown
Member Author

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

@panva panva removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 25, 2026
@IlyasShabi IlyasShabi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 25, 2026
@IlyasShabi IlyasShabi requested review from Qard and targos March 25, 2026 15:32
@IlyasShabi
Copy link
Copy Markdown
Member Author

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

@targos
Copy link
Copy Markdown
Member

targos commented Mar 26, 2026

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

@IlyasShabi
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member

joyeecheung commented Mar 26, 2026

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
Copy link
Copy Markdown
Member Author

IlyasShabi commented Mar 26, 2026

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?

@aduh95 aduh95 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 27, 2026
@IlyasShabi
Copy link
Copy Markdown
Member Author

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@IlyasShabi IlyasShabi force-pushed the ishabi/v8-islive-sample branch from 2806f85 to 83e86e3 Compare April 23, 2026 08:41
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@IlyasShabi IlyasShabi added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@IlyasShabi IlyasShabi added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@BridgeAR
Copy link
Copy Markdown
Member

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

@RafaelGSS
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

@IlyasShabi
Copy link
Copy Markdown
Member Author

bengl pushed a commit that referenced this pull request Apr 28, 2026
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 <[email protected]>
    Commit-Queue: Dominik Inführ <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105698}

Refs: v8/v8@0f024d4
Signed-off-by: ishabi <[email protected]>
PR-URL: #62408
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Copy Markdown
Member

Landed in 8385efc

@BridgeAR BridgeAR closed this Apr 28, 2026
@joyeecheung
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member

richardlau commented Apr 28, 2026

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
Copy link
Copy Markdown
Member Author

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

@IlyasShabi IlyasShabi deleted the ishabi/v8-islive-sample branch April 28, 2026 20:39
@joyeecheung
Copy link
Copy Markdown
Member

joyeecheung commented Apr 30, 2026

https://github.com/nodejs/reliability/blob/main/reports/2026-04-30.md FWIW it seems the flakes also mysteriously stopped showing up (i.e. the count is not increasing..)

RafaelGSS pushed a commit that referenced this pull request May 4, 2026
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 <[email protected]>
    Commit-Queue: Dominik Inführ <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105698}

Refs: v8/v8@0f024d4
Signed-off-by: ishabi <[email protected]>
PR-URL: #62408
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
RafaelGSS added a commit that referenced this pull request May 4, 2026
assert:
  * (SEMVER-MAJOR) allow printf-style messages as assertion error (Ruben Bridgewater) #58849
build:
  * (SEMVER-MAJOR) bump GCC requirement to 13.2 (Michaël Zasso) #62555
  * (SEMVER-MAJOR) enable Temporal by default (Richard Lau) #61806
  * (SEMVER-MAJOR) enable V8_VERIFY_WRITE_BARRIERS in debug build (Joyee Cheung) #61898
  * (SEMVER-MAJOR) reset embedder string to "-node.0" (Michaël Zasso) #61898
  * (SEMVER-MAJOR) target Power 9 for AIX/IBM i (Richard Lau) #62296
  * (SEMVER-MAJOR) drop support for Python 3.9 (Mike McCready) #61177
  * (SEMVER-MAJOR) enable maglev for Linux on s390x (Richard Lau) #60863
  * (SEMVER-MAJOR) reset embedder string to "-node.0" (Michaël Zasso) #60488
  * (SEMVER-MAJOR) reset embedder string to "-node.0" (Michaël Zasso) #60111
build,test:
  * (CVE-2026-21717) (SEMVER-MAJOR) test array index hash collision (Joyee Cheung) #61898
build,win:
  * (SEMVER-MAJOR) fix Temporal build (StefanStojanovic) #61806
crypto:
  * (SEMVER-MAJOR) unify asymmetric key import through KeyObjectHandle::Init (Filip Skokan) #62499
  * (SEMVER-MAJOR) runtime-deprecate DEP0203 and DEP0204 (Filip Skokan) #62453
  * (SEMVER-MAJOR) decorate async crypto job errors with OpenSSL error details (Filip Skokan) #62348
  * (SEMVER-MAJOR) default ML-KEM and ML-DSA pkcs8 export to seed-only format (Filip Skokan) #62178
  * (SEMVER-MAJOR) move DEP0182 to End-of-Life (Tobias Nießen) #61084
  * (SEMVER-MAJOR) fix DOMException name for non-extractable key error (Filip Skokan) #60830
deps:
  * (SEMVER-MAJOR) V8: cherry-pick 0f024d4e66e0 (ishabi) #62408
  * (SEMVER-MAJOR) fix V8 race condition for AIX (Abdirahim Musse) #61898
  * (SEMVER-MAJOR) V8: cherry-pick cd2c216e7658 (LuYahan) #61898
  * (SEMVER-MAJOR) V8: backport 088b7112e7ab (Igor Sheludko) #61898
  * (SEMVER-MAJOR) V8: cherry-pick 00f6e834029f (Joyee Cheung) #61898
  * (SEMVER-MAJOR) V8: backport bef0d9c1bc90 (Joyee Cheung) #61898
  * (SEMVER-MAJOR) V8: cherry-pick cf1bce40a5ef (Richard Lau) #61898
  * (SEMVER-MAJOR) V8: cherry-pick daf4656ba85e (Milad Fa) #61898
  * (SEMVER-MAJOR) V8: cherry-pick d83f479604c8 (Joyee Cheung) #61898
  * (SEMVER-MAJOR) V8: cherry-pick edeb0a4fa181 (Joyee Cheung) #61898
  * (SEMVER-MAJOR) V8: cherry-pick aa0b288f87cc (Richard Lau) #61898
  * (SEMVER-MAJOR) patch V8 to fix Windows build (StefanStojanovic) #61898
  * (SEMVER-MAJOR) V8: cherry-pick highway@989a498fdf3 (Richard Lau) #61898
  * (SEMVER-MAJOR) support madvise(3C) across ALL illumos revisions (Dan McDonald) #61898
  * (SEMVER-MAJOR) patch V8 for illumos (Dan McDonald) #61898
  * (SEMVER-MAJOR) remove problematic comment from v8-internal (Michaël Zasso) #61898
  * (SEMVER-MAJOR) define V8_PRESERVE_MOST as no-op on Windows (Stefan Stojanovic) #61898
  * (SEMVER-MAJOR) patch V8 to avoid duplicated zlib symbol (Michaël Zasso) #61898
  * (SEMVER-MAJOR) update V8 to 14.6.202.33 (Michaël Zasso) #61898
  * (SEMVER-MAJOR) update undici to 8.0.2 (Node.js GitHub Bot) #62384
  * (SEMVER-MAJOR) V8: backport 151d0a44a1b2 (Abdirahim Musse) #60488
  * (SEMVER-MAJOR) V8: cherry-pick 47800791b35c (Jakob Kummerow) #60488
  * (SEMVER-MAJOR) patch V8 for illumos (Dan McDonald) #59805
  * (SEMVER-MAJOR) use std::map in MSVC STL for EphemeronRememberedSet (Joyee Cheung) #58070
  * (SEMVER-MAJOR) remove problematic comment from v8-internal (Michaël Zasso) #58070
  * (SEMVER-MAJOR) patch V8 to avoid duplicated zlib symbol (Michaël Zasso) #54077
  * (SEMVER-MAJOR) update V8 to 14.3.127.12 (Michaël Zasso) #60488
  * (SEMVER-MAJOR) V8: cherry-pick ff34ae20c8e3 (Chengzhong Wu) #60111
  * (SEMVER-MAJOR) V8: backport fed47445bbdd (Abdirahim Musse) #60111
  * (SEMVER-MAJOR) patch V8 for illumos (Dan McDonald) #59805
  * (SEMVER-MAJOR) use std::map in MSVC STL for EphemeronRememberedSet (Joyee Cheung) #58070
  * (SEMVER-MAJOR) remove problematic comment from v8-internal (Michaël Zasso) #58070
  * (SEMVER-MAJOR) patch V8 to avoid duplicated zlib symbol (Michaël Zasso) #54077
  * (SEMVER-MAJOR) update V8 to 14.2.231.9 (Michaël Zasso) #60111
diagnostics_channel:
  * (SEMVER-MAJOR) ensure tracePromise consistency with non-Promises (René) #61766
doc:
  * (SEMVER-MAJOR) remove extensionless CJS exception for type:module packages (Matteo Collina) #62176
  * (SEMVER-MAJOR) update supported Windows SDK version to 11 (Mike McCready) #61973
  * (SEMVER-MAJOR) drop p8 and z13 support (Milad Fa) #61005
http:
  * (SEMVER-MAJOR) move writeHeader to end-of-life (Sebastian Beltran) #60635
  * (SEMVER-MAJOR) fix handling of HTTP upgrades with bodies (Tim Perry) #60016
lib:
  * (SEMVER-MAJOR) return undefined for localStorage without file (Matteo Collina) #61333
lib,src:
  * (SEMVER-MAJOR) implement QuotaExceededError as DOMException-derived interface (Filip Skokan) #62293
module:
  * (SEMVER-MAJOR) runtime-deprecate module.register() (Geoffrey Booth) #62401
  * (SEMVER-MAJOR) remove --experimental-transform-types (Marco Ippolito) #61803
src:
  * (SEMVER-MAJOR) replace uses of deprecated v8::External APIs (gahaas) #61898
  * (SEMVER-MAJOR) stop using `v8::PropertyCallbackInfo<T>::This()` (Igor Sheludko) #61898
  * (SEMVER-MAJOR) avoid deprecated Wasm API (Clemens Backes) #61898
  * (SEMVER-MAJOR) avoid deprecated `FixedArray::Get` (Clemens Backes) #61898
  * (SEMVER-MAJOR) update NODE_MODULE_VERSION to 147 (Michaël Zasso) #61898
  * (SEMVER-MAJOR) remove deprecated and unused isolate fields (Michaël Zasso) #60488
  * (SEMVER-MAJOR) update NODE_MODULE_VERSION to 144 (Michaël Zasso) #60488
  * (SEMVER-MAJOR) include `node_api_types.h` instead of `node_api.h` in `node.h` (Anna Henningsen) #60496
  * (SEMVER-MAJOR) update NODE_MODULE_VERSION to 142 (Michaël Zasso) #60111
stream:
  * (SEMVER-MAJOR) promote DEP0201 to runtime deprecation (René) #62173
  * (SEMVER-MAJOR) move _stream_* to end-of-life (Sebastian Beltran) #60657
  * (SEMVER-MAJOR) readable read one buffer at a time (Robert Nagy) #60441
  * (SEMVER-MAJOR) preserve AsyncLocalStorage on finished only when needed (avcribl) #59873
test:
  * (SEMVER-MAJOR) skip wasm allocation tests in workers (Michaël Zasso) #61898
  * (SEMVER-MAJOR) update wpt Wasm jsapi expectations (Michaël Zasso) #61898
  * (SEMVER-MAJOR) support presence of Temporal global (Michaël Zasso) #61898
  * (SEMVER-MAJOR) add type tags to uses of v8::External (gahaas) #61898
  * (SEMVER-MAJOR) fix test-linux-perf-logger for V8 14.3 (Michaël Zasso) #60488
tools:
  * (SEMVER-MAJOR) remove v8_initializers_slow workaround from v8.gyp (Michaël Zasso) #61898
  * (SEMVER-MAJOR) add Rust args to `tools/make-v8.sh` (Richard Lau) #61898
  * (SEMVER-MAJOR) update V8 gypfiles for 14.6 (Michaël Zasso) #61898
  * (SEMVER-MAJOR) update V8 gypfiles for 14.5 (Michaël Zasso) #61898
  * (SEMVER-MAJOR) update V8 gypfiles for 14.4 (Michaël Zasso) #61898
util:
  * (SEMVER-MAJOR) mark proxied objects as such when inspecting them (Ruben Bridgewater) #61029
  * (SEMVER-MAJOR) reduce TextEncoder.encodeInto function size (Yagiz Nizipli) #60339

PR-URL: #62526
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants