Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: reset process.versions during pre-execution #53444

Merged
merged 1 commit into from
Jun 20, 2024

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
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2cb3504

@richardlau richardlau deleted the resetprocessversions branch June 20, 2024 18:09
sophoniie pushed a commit to sophoniie/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