-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
Cache accesses to process.env? #3104
Comments
See #1648. |
Thanks @ChALkeR. I understand the argument for keeping it as-is, but if it is kept this way, I think it would help to document that values should be cached whenever possible as serious performance problems can result. Alternatively, a function like |
Is it actually possible to change the env externally once the process is running? If not, why not cache this? |
@Fishrock123 Any native module (or a library that is used by it) could call
-1 on caching, +1 on the minor note in the docs. While we are here, the docs for the |
@silverwind It's really more of a problem with child modules, like React itself (which is finally caching I think @ChALkeR has the right approach; I underestimated how much potential breakage there is. But documenting it properly and making it known that Perhaps it should have been function getter/setters, which would have been more expected to have slow behavior. Too late now of course. |
If that was the only possible way to set the env from node, then it could be covered by updating the cache in the setter. But it's not. |
Doesn't seem worthwhile. Closing, please cache it yourself if need be. :) |
Just wanted to ping this, we're having quite a hard time handling this in React, and caching NODE_ENV results in a 100% (!) speedup in benchmark code. In real-world testing, I see roughly 50-70%. It's significant. I'm getting sick of issuing PRs to repos caching access to NODE_ENV, and there really isn't a good solution for modules that need to hide features behind debug flags and want them eliminated in bundling. If we cache the environment lookup, UglifyJS is unable to eliminate the dead code in bundling unless we use a In reality, is it really useful to rely on external changes to the env (say, from native code)? What proportion of users really need this, and is it a valuable tradeoff compared to the performance benefits we could get ecosystem-wide? Would the Node core contributors consider a semver-major patch? My thoughts are to make |
@STRML could you detail what problems you are having caching it locally? |
@STRML Another option, make a module that does this and load it in const env = process.env
delete process.env // might not be necessary
process.env = env |
I've been doing: process.env = JSON.parse(JSON.stringify(process.env)); Which seems to work fine. Okay - so the issue with a library like React caching locally is that there are three main usage paths and two goals. The goals:
As a library author, you usually deliver pre-bundled source (dist.js, dist.min.js), built source (say, with babel, in a There are three main usage paths:
// This would work if it were a const
var NODE_ENV = process.env.NODE_ENV;
//...
if(NODE_ENV !== 'production') { // this will not be eliminated
// debug code
} Closure compiler has a As a result, if we solve Path 2 (NodeJS), we produce larger bundles for Path 3 (Webpack/Browserify) with UglifyJS. In the case of React, the bundle can be as much as 40% larger gzipped, so it's very significant (38 vs 53kb). If |
Is there a real reason to use env for this? You can use global.__REACT_ENV the same way |
It's possible. It appears to be the attitude of the devs that they want to avoid polluting globals whenever possible. Despite that, this is a more widespread problem that doesn't just affect React. |
The performance penalty is still not mentioned in the docs. On the contrary, because changes don't affect the shell, the docs seem to imply that env is just like any other JS object. |
They do, child processes created with the child_process module inherit the modified environment unless told otherwise. Are you thinking of seeing changes to |
Sorry, I'm not much of a shell programmer. I just would rather not have to discover the performance penalty when it's a problem. Why can't the docs say reading process.env makes system calls (if that's what it's doing)? Why do we have to discover that process.env is not a normal JS object the hard way? |
We accept pull requests, you're welcome to have a stab at improving the documentation. Having said that, any JS object can be "magic" and really be a bunch of C++ code; |
I'd have to investigate the node.js code to have confidence making a statement. Several people in this discussion are already claiming to know how it works. I'm hoping that someone who wants to be helpful wouldn't mind taking a minute to update the repo they already have checked out. |
Just a small documentation update, if you're open to it. Suggesting that `NODE_ENV` be "cached" in the example `unsafeValidate` method. Reasoning / discussions here: * nodejs/node#3104 * nodejs/node#1648 Unless you'd expect `NODE_ENV` to be editable while the application is running (debugging on a live server, perhaps?)
It looks like Node is caching
|
I'd like to warn you that previous comment is not the case. Node.js does not cache env vars, so each time you use process.env.VAR you actually call getnev(). This might be a performance issue if you have lots of env vars (this is especially important in case you run your node.js app in k8s since k8s creates env vars for all endpoints in a namespace, see kubernetes/kubernetes#60099 for details) Proof (running node.js which prints some env var in an endless loop, attaching to the node.js process via gdb and changing this var):
|
Removes the 'log' export from 'insight'. Accessing process.env.XXX is relatively slow in Node.js. Benchmark of a plain object property access and accessing process.env.NODE_ENV: ``` property access 500000000 iterations 0 ns/op process.env access 5000000 iterations 246 ns/op ``` See this thread nodejs/node#3104 for some more context
Removes the 'log' export from 'insight'. Accessing process.env.XXX is relatively slow in Node.js. Benchmark of a plain object property access and accessing process.env.NODE_ENV: ``` property access 500000000 iterations 0 ns/op process.env access 5000000 iterations 246 ns/op ``` See this thread nodejs/node#3104 for some more context
Highlight user code in internal stacktrace. Other Changes Avoid to use process.env at runtime because it does an expensive syscall (CF: nodejs/node#3104 (comment)) Boyscout export Global in a TS file to avoid the necessity of building the app to run it
I'm writing here just to clarify this fundamental misunderstanding of how UNIX Environments work, for people who read this in the future (I came to this from a thread on an internet forum). The Shell Environment is defined in the SYSV ABI Specification, next to or in the same section where the auxillary ( Typically, because of it's inherent immutability (disregarding e.g. if you want to communicate an environment to your child, then pass an environment explicitly to your child; if you want to communicate to other threads, keep in mind that EDIT: An addendum. One typical example of wanting to use
and then in the child:
However this is a security risk detailed in the CERT C Coding Guidelines (Which are equally applicable in this instance! Probably one of the few instances where the semantics of Windows/Linux, C, POSIX, and Javascript are all applicable !). https://wiki.sei.cmu.edu/confluence/display/c/ENV03-C.+Sanitize+the+environment+when+invoking+external+programs and https://wiki.sei.cmu.edu/confluence/display/c/ENV02-C.+Beware+of+multiple+environment+variables+with+the+same+effective+name :) |
I've found in multiple projects that accessing
process.env
within a hot section of the code leads to major slowdown.This really hurts in React server-side rendering (issue) and has caused them to rearrange how they access the env.
It would sense to cache already-accessed properties rather than reaching out to the actual environment. And of course, update the cache on assignment.
I would be happy to contribute a patch if anyone could point me in the right direction to get started, as I'm new to Node core dev.
The text was updated successfully, but these errors were encountered: