Skip to content

Conversation

richardlau
Copy link
Member

Reset process.versions during pre-execution so that it reflects the versions at run-time rather than when the snapshot was taken.

Fixes: #51007 (comment)


I'm not sure there's an easy way to test this currently in the CI -- it requires Node.js to be built against a dynamically linked library (e.g. zlib[1]) at one version and then run against a different, ABI-compatible version so that the snapshot has one version of the dependency recorded but Node.js is actually running with a different one.

cc @joyeecheung -- re. #51007 (comment) -- it seemed easier to reset the entire process.versions object rather than checking each individual entry (and also meant I could extract and reuse the setting code).

[1]: OpenSSL would be another, but there's a separate bug with obtaining the version of OpenSSL that I'll open another PR for.

Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 13, 2024
@joyeecheung
Copy link
Member

Slightly concerned about the runtime overhead of this, since the process.versions have quite a few entries now, but it might not be a big deal, started https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1569/ to find out

@richardlau
Copy link
Member Author

Slightly concerned about the runtime overhead of this, since the process.versions have quite a few entries now, but it might not be a big deal, started https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1569/ to find out

19:12:40                                                                                           confidence improvement accuracy (*)   (**)  (***)
19:12:40 misc/startup-core.js count=30 mode='process' script='benchmark/fixtures/require-builtins'          *     -0.36 %       ±0.31% ±0.42% ±0.54%
19:12:40 misc/startup-core.js count=30 mode='process' script='test/fixtures/semicolon'                             0.65 %       ±3.07% ±4.08% ±5.31%
19:12:40 misc/startup-core.js count=30 mode='process' script='test/fixtures/snapshot/typescript'          ***      0.28 %       ±0.04% ±0.06% ±0.08%
19:12:40 misc/startup-core.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'         ***     -0.47 %       ±0.24% ±0.33% ±0.42%
19:12:40 misc/startup-core.js count=30 mode='worker' script='test/fixtures/semicolon'                             -0.16 %       ±0.46% ±0.61% ±0.79%
19:12:40 misc/startup-core.js count=30 mode='worker' script='test/fixtures/snapshot/typescript'                    0.17 %       ±0.41% ±0.54% ±0.71%
19:12:40 
19:12:40 Be aware that when doing many comparisons the risk of a false-positive
19:12:40 result increases. In this case, there are 6 comparisons, you can thus
19:12:40 expect the following amount of false-positive results:
19:12:40   0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
19:12:40   0.06 false positives, when considering a   1% risk acceptance (**, ***),
19:12:40   0.01 false positives, when considering a 0.1% risk acceptance (***)

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

@joyeecheung does the benchmark result address your overhead concerns?

@joyeecheung
Copy link
Member

joyeecheung commented Jun 18, 2024

It does show that this has a performance impact although it’s limited so far - but might be bigger as we continue to add dependencies. Ideally I think it would be better to reset the object via a snapshotted v8::ObjectTemplate with lazy data properties (so the specific version entries will be slower to access on first read but otherwise the performance overhead of creating a new object is marginal). But that can be done in a follow up.

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 20, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2cb3504 into nodejs:main Jun 20, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 2cb3504

@richardlau richardlau deleted the resetprocessversions branch June 20, 2024 18:09
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.

PR-URL: nodejs#53444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Jun 21, 2024
Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.

PR-URL: #53444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.

PR-URL: nodejs#53444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.

PR-URL: #53444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
Reset `process.versions` during pre-execution so that it reflects
the versions at run-time rather than when the snapshot was taken.

PR-URL: #53444
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jul 25, 2024
codebytere added a commit to electron/electron that referenced this pull request Jul 26, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request Jul 26, 2024
* chore: bump node in DEPS to v20.16.0

* test: skip unstable shadow realm gc tests

nodejs/node#52855

* test: extend env for `test-node-output-errors`

nodejs/node#53535

* src: fix typo in env.cc

nodejs/node#53418

* src: reset `process.versions` during pre-execution

nodejs/node#53444

* chore: fixup patch indices

* src,permission: --allow-wasi & prevent WASI exec

nodejs/node#53124

* tls: use SSL_get_peer_tmp_key

nodejs/node#53366

* deps: update c-ares to 1.29.0

nodejs/node#53155

* src: account for OpenSSL unexpected version

* crypto: fix propagation of "memory limit exceeded"

nodejs/node#53300

* process: add process.getBuiltinModule(id)

nodejs/node#52762

* windows 32bit: config change callback needs to be stdcall

c-ares/c-ares@8f265c9

* fix: building with UNICODE

c-ares/c-ares#802

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: Keeley Hammond <khammond@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants