src: migrate from deprecated SnapshotCreator constructor by joyeecheung · Pull Request #55337 · nodejs/node

joyeecheung · 2024-10-09T16:50:00Z

Previously we have been using the variant of SnapshotCreator that only passes the external references instead of
v8::Isolate::CreateParams and it's about to be deprecated. Switch to using the new constructor that takes a fully CreateParams instead.

This also makes sure that the snapshot building script is using the Node.js array buffer allocator instead of a separate default one that was previously used by the old constructor. The zero fill toggle in the Node.js array buffer allocator would still be ignored during snapshot building, however, until we fixes the array buffer allocator and let V8 own the toggle backing store instead, because otherwise the snapshot would contain the external toggle address and become unreproducible.

nodejs-github-bot · 2024-10-09T16:50:05Z

Review requested:

@nodejs/startup

codecov · 2024-10-09T18:12:44Z

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (29af9ce) to head (b007f46).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/buffer.js	72.22%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55337   +/-   ##
=======================================
  Coverage   90.24%   90.24%           
=======================================
  Files         630      630           
  Lines      185670   185688   +18     
  Branches    36405    36407    +2     
=======================================
+ Hits       167555   167574   +19     
  Misses      10998    10998           
+ Partials     7117     7116    -1

Files with missing lines	Coverage Δ
lib/internal/process/pre_execution.js	`95.96% <ø> (+0.40%)`	⬆️
src/api/embed_helpers.cc	`75.98% <100.00%> (+1.36%)`	⬆️
src/node_buffer.cc	`67.20% <100.00%> (-0.31%)`	⬇️
src/node_snapshotable.cc	`75.52% <100.00%> (+0.13%)`	⬆️
lib/internal/buffer.js	`98.49% <72.22%> (-0.26%)`	⬇️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

legendecas · 2024-10-10T07:05:28Z

I assume the labels were removed because of the awkward GitHub UI.

lib/internal/buffer.js

joyeecheung · 2024-10-25T13:35:08Z

It seems the checksum of the read only snapshots are mismatched after the changes. Need to look into a bit..

github-actions · 2025-04-12T23:27:48Z

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - src: migrate from deprecated SnapshotCreator constructor
   ⚠  - fixup! src: migrate from deprecated SnapshotCreator constructor
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14424311872

Previously we have been using the variant of SnapshotCreator that only passes the external references instead of v8::Isolate::CreateParams and it's about to be deprecated. Switch to using the new constructor that takes a fully CreateParams instead. This also makes sure that the snapshot building script is using the Node.js array buffer allocator instead of a separate default one that was previously used by the old constructor. The zero fill toggle in the Node.js array buffer allocator would still be ignored during snapshot building, however, until we fixes the array buffer allocator and let V8 own the toggle backing store instead, because otherwise the snapshot would contain the external toggle address and become unreproducible.

nodejs-github-bot · 2025-04-14T16:52:12Z

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

Previously we have been using the variant of SnapshotCreator that only passes the external references instead of v8::Isolate::CreateParams and it's about to be deprecated. Switch to using the new constructor that takes a fully CreateParams instead. This also makes sure that the snapshot building script is using the Node.js array buffer allocator instead of a separate default one that was previously used by the old constructor. The zero fill toggle in the Node.js array buffer allocator would still be ignored during snapshot building, however, until we fixes the array buffer allocator and let V8 own the toggle backing store instead, because otherwise the snapshot would contain the external toggle address and become unreproducible. PR-URL: #55337 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2025-04-14T19:12:22Z

Landed in b2405e9

Previously we have been using the variant of SnapshotCreator that only passes the external references instead of v8::Isolate::CreateParams and it's about to be deprecated. Switch to using the new constructor that takes a fully CreateParams instead. This also makes sure that the snapshot building script is using the Node.js array buffer allocator instead of a separate default one that was previously used by the old constructor. The zero fill toggle in the Node.js array buffer allocator would still be ignored during snapshot building, however, until we fixes the array buffer allocator and let V8 own the toggle backing store instead, because otherwise the snapshot would contain the external toggle address and become unreproducible. PR-URL: #55337 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

lib/internal/buffer.js

ChALkeR · 2025-10-27T04:12:06Z

I checked the following:

4968db1 zero-fills allocUnsafe, 4968db1^ doesn't
494669b zero-fills allocUnsafe, 494669b^ doesn't
1e4871c zero-fills allocUnsafe, 1e4871c^ too, 1e4871c^^ doesn't

b2405e9 zero-fills allocUnsafe, b2405e9^ doesn't

b2405e9 was landed

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 9, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2024

This comment was marked as outdated.

Sign in to view

legendecas approved these changes Oct 10, 2024

View reviewed changes

legendecas added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 10, 2024

vmoroz reviewed Oct 14, 2024

View reviewed changes

lib/internal/buffer.js Show resolved Hide resolved

vmoroz approved these changes Oct 15, 2024

View reviewed changes

addaleax approved these changes Oct 18, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2024

This comment was marked as outdated.

Sign in to view

richardlau mentioned this pull request Oct 18, 2024

V8 13.0 Deprecations #55452

Open

7 tasks

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Oct 25, 2024

joyeecheung mentioned this pull request Apr 10, 2025

[help wanted] deps: update V8 to 13.6 #57753

Closed

joyeecheung force-pushed the snapshot-params branch from 4968db1 to 494669b Compare April 12, 2025 18:17

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025

This comment was marked as outdated.

Sign in to view

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Apr 12, 2025

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 12, 2025

This comment was marked as outdated.

Sign in to view

joyeecheung removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 13, 2025

joyeecheung added 2 commits April 14, 2025 13:34

fixup! src: migrate from deprecated SnapshotCreator constructor

b007f46

joyeecheung force-pushed the snapshot-params branch from 1e4871c to b007f46 Compare April 14, 2025 12:07

This comment was marked as outdated.

Sign in to view

joyeecheung added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Apr 14, 2025

jasnell approved these changes Apr 14, 2025

View reviewed changes

jasnell closed this Apr 14, 2025

RafaelGSS mentioned this pull request May 1, 2025

2025-05-06, Version 24.0.0 (Current) #57609

Merged

ChALkeR reviewed Oct 27, 2025

View reviewed changes

lib/internal/buffer.js Show resolved Hide resolved

ChALkeR mentioned this pull request Oct 27, 2025

allocUnsafe is useless since 24 #60423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: migrate from deprecated SnapshotCreator constructor#55337

src: migrate from deprecated SnapshotCreator constructor#55337
joyeecheung wants to merge 2 commits intonodejs:mainfrom
joyeecheung:snapshot-params

joyeecheung commented Oct 9, 2024

Uh oh!

nodejs-github-bot commented Oct 9, 2024

Uh oh!

This comment was marked as outdated.

codecov bot commented Oct 9, 2024 •

edited

Loading

Uh oh!

legendecas commented Oct 10, 2024

Uh oh!

Uh oh!

This comment was marked as outdated.

joyeecheung commented Oct 25, 2024

Uh oh!

This comment was marked as outdated.

github-actions bot commented Apr 12, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 14, 2025 •

edited by jasnell

Loading

Uh oh!

jasnell commented Apr 14, 2025

Uh oh!

Uh oh!

ChALkeR commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

joyeecheung commented Oct 9, 2024

Uh oh!

nodejs-github-bot commented Oct 9, 2024

Uh oh!

This comment was marked as outdated.

codecov bot commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

legendecas commented Oct 10, 2024

Uh oh!

Uh oh!

This comment was marked as outdated.

joyeecheung commented Oct 25, 2024

Uh oh!

This comment was marked as outdated.

github-actions bot commented Apr 12, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Apr 14, 2025 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 14, 2025

Uh oh!

Uh oh!

ChALkeR commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Oct 9, 2024 •

edited

Loading

nodejs-github-bot commented Apr 14, 2025 •

edited by jasnell

Loading