-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: use libuv to get env vars #29188
Conversation
If the drop in enumeration performance is considered an issue for this to happen, I’d probably try to add a callback-based enumeration API to libuv. |
CI: https://ci.nodejs.org/job/node-test-pull-request/25057/ ❌ (Windows test failures seem relevant?) |
I think this is blocked on libuv/libuv#2419 (which, ironically, solves a bug that Node.js also had in the past). I’ve pushed changes for the linter failure. |
This fixes #27211 right? |
@joyeecheung Yeah, it does … I had kind of forgotten that #27310 existed. Should we rebase that one first? It doesn’t take care of everything that this PR does, but most of it. |
I think either rebasing or keeping this PR works considering #27310 has been inactive for 2 months, but I'll leave it to you to decide :) |
CI: https://ci.nodejs.org/job/node-test-pull-request/25178/ ❌ (Windows test failures are relevant) |
@@ -3,15 +3,55 @@ | |||
const common = require('../common'); | |||
|
|||
const bench = common.createBenchmark(main, { | |||
n: [1e5], | |||
n: [1e6], | |||
operation: ['get', 'set', 'enumerate', 'query', 'delete'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change means we need to add a line for operation
in test/benchmark/test-benchmark-process.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Trott Thanks for catching, done!
feb6d8f
to
ce47850
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows failures in CI seem relevant. Feel free to dismiss this when tests are fixed or if it is determined that I'm wrong and they're not relevant--no need to ask me or anything. Just putting this here so no one over-ambitious tries to land the PR without that getting resolved.
Windows CI should be better now, see libuv/libuv#2473 for the bug behind it (which, luckily, is easy to work around here). |
Wondering if the failures are due to using "Rebuild" for some reason. Will start a fresh CI from "Build With Parameters" instead and we'll see what happens.... |
Quite likely. "Rebuild" only reruns jobs that have failed. In the case of Windows (and ARM) the compile and test jobs are separate so "rebuild" will not recompile code if the compile job in the original CI passed (even if the code was changed in between the original CI and the rebuild). |
Ok, looks like the Windows failures are related again. I’ll take a look. |
Benchmark different types of operations and make results comparable by normalizing process.env for enumeartion.
This allows us to remove OS-dependent code. confidence improvement accuracy (*) (**) (***) process/bench-env.js operation='delete' n=1000000 3.57 % ±10.86% ±14.46% ±18.85% process/bench-env.js operation='enumerate' n=1000000 *** -14.06 % ±7.46% ±9.94% ±12.96% process/bench-env.js operation='get' n=1000000 -7.97 % ±11.80% ±15.70% ±20.45% process/bench-env.js operation='query' n=1000000 -1.32 % ±8.38% ±11.17% ±14.58% process/bench-env.js operation='set' n=1000000 -0.98 % ±9.63% ±12.81% ±16.68% The drop in enumeration performance is likely due to the large number of extra allocations that libuv performs. However, enumerating process.env should generally not be a hot path in most applications.
347f340
to
eed2884
Compare
Rebased & should have fixed the issue that lead to CI failing, which was that the skipped, unenumerable env vars on Windows were counted towards the length of the resulting array, even though there were no corresponding entries in the array. |
Benchmark different types of operations and make results comparable by normalizing process.env for enumeartion. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This allows us to remove OS-dependent code. confidence improvement accuracy (*) (**) (***) process/bench-env.js operation='delete' n=1000000 3.57 % ±10.86% ±14.46% ±18.85% process/bench-env.js operation='enumerate' n=1000000 *** -14.06 % ±7.46% ±9.94% ±12.96% process/bench-env.js operation='get' n=1000000 -7.97 % ±11.80% ±15.70% ±20.45% process/bench-env.js operation='query' n=1000000 -1.32 % ±8.38% ±11.17% ±14.58% process/bench-env.js operation='set' n=1000000 -0.98 % ±9.63% ±12.81% ±16.68% The drop in enumeration performance is likely due to the large number of extra allocations that libuv performs. However, enumerating process.env should generally not be a hot path in most applications. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Benchmark different types of operations and make results comparable by normalizing process.env for enumeartion. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This allows us to remove OS-dependent code. confidence improvement accuracy (*) (**) (***) process/bench-env.js operation='delete' n=1000000 3.57 % ±10.86% ±14.46% ±18.85% process/bench-env.js operation='enumerate' n=1000000 *** -14.06 % ±7.46% ±9.94% ±12.96% process/bench-env.js operation='get' n=1000000 -7.97 % ±11.80% ±15.70% ±20.45% process/bench-env.js operation='query' n=1000000 -1.32 % ±8.38% ±11.17% ±14.58% process/bench-env.js operation='set' n=1000000 -0.98 % ±9.63% ±12.81% ±16.68% The drop in enumeration performance is likely due to the large number of extra allocations that libuv performs. However, enumerating process.env should generally not be a hot path in most applications. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Benchmark different types of operations and make results comparable by normalizing process.env for enumeartion. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This allows us to remove OS-dependent code. confidence improvement accuracy (*) (**) (***) process/bench-env.js operation='delete' n=1000000 3.57 % ±10.86% ±14.46% ±18.85% process/bench-env.js operation='enumerate' n=1000000 *** -14.06 % ±7.46% ±9.94% ±12.96% process/bench-env.js operation='get' n=1000000 -7.97 % ±11.80% ±15.70% ±20.45% process/bench-env.js operation='query' n=1000000 -1.32 % ±8.38% ±11.17% ±14.58% process/bench-env.js operation='set' n=1000000 -0.98 % ±9.63% ±12.81% ±16.68% The drop in enumeration performance is likely due to the large number of extra allocations that libuv performs. However, enumerating process.env should generally not be a hot path in most applications. PR-URL: #29188 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
A small investigation took me to this pull-request. I have a quick question: Any reason to why we are not using |
benchmark: improve process.env benchmarks
Benchmark different types of operations and make results comparable
by normalizing process.env for enumeartion.
src: use libuv to get env vars
This allows us to remove OS-dependent code.
The drop in enumeration performance is likely due to the large
number of extra allocations that libuv performs. However, enumerating
process.env should generally not be a hot path in most applications.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes