You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
vm.compileFunction(source) is much slower than new vm.Script(source) #35375
Platform: Darwin Simens-MacBook-Pro.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Aug 31 22:12:52 PDT 2020; root:xnu-6153.141.2~1/RELEASE_X86_64 x86_64
It's just yarn init -2 to fetch the bundled yarn v2 source code (to have a large JS file), then loading that into new Script and compileFunction 100 times and seeing how long it takes.
Running this prints the following on my machine (MBP 2018):
Running with Script took 50 ms
Running with compileFunction took 4041 ms
How often does it reproduce? Is there a required condition?
Every time
What is the expected behavior?
It should be comparably fast to pass source code into compileFunction as it is to pass it into new Script.
What do you see instead?
About 80x worse performance. It gets worse the larger the loop is, so I'm assuming it's "simply" some cached data so new Script avoids the hit of parsing the same code multiple times, as it's fairly constant regardless of loop iterations
As mentioned in #26229 (comment) we see a huge performance improvement in Jest if we switch from compileFunction to the "old" new Script approach. As also mentioned there, this might be "fixed" (or at least possible to solve in userland) by the seemingly stalled #24069?
I added a quick and dirty produceCachedData, and it went down to about 180 ms. Which is a great improvement, albeit still 5x slower than new Script. And harder to manage (I'd prefer the implicit caching of the new Script APIs)
This is about v8::ScriptCompiler::CompileFunctionInContext() not using the code cache, right?
I don't think that's V8's issue to fix. V8 can't know whether it's safe to use the cache because the argument names and context extensions affect how the function is parsed.
Not that it's easy for Node.js. options.contextExtensions is a free-form list of mutable objects. Hard to cache on that unless object identity is good enough - but it probably isn't.
What Node.js (or perhaps V8) could optimize for is the presumably common case of no context extensions. TBD how to cache without introducing memory leaks.
In Jest's case we'll be passing a different parsingContext all the time (Jest uses a different Context per test file for sandboxing), so if the cached data is dependent on the Context we'd probably still get a painful performance regression vs using vm.Script. If it's not possible to get caching based on the source code string (and the arguments) alone, we'll probably have to drop usage of compileFunction and stay on vm.Script.
Using the args array to vary the cache is no issue for our use case, though - 99% of the time they'll be the same, and for the cases it's not I guess it'd just create some more? Same as we have today with the manual function(some, args) {USER_SOURCE_CODE_HERE} wrapper we add which changes its source code via the arguments we pass in the manual function wrapper.
(That said, improving performance for compileFunction in the case of not passing parsingContext seems like a good thing even if it wouldn't be enough for Jest's use case 🙂)
If it's not possible to get caching based on the source code string (and the arguments) alone
I don't think that can work so you're probably better off sticking with vm.Script, yes. Argument names aren't insurmountable but context extensions are.
When you pass in context extensions, what's evaluated looks like this in pseudo code:
// assume a = { y: 42 } and b = { z: 1337 }with(a)with(b)functionf(x){returnx*y*z}
I say "pseudo code" but in fact it's really close to the runtime representation.
@bnoordhuis reading your comment more closely, I see you're talking about contextExtensions. We don't use that, we use parsingContext - does that have the same limitations you think?
Fwiw, Node 16 has both Script and compileFunction performing similarly:
rishi vm-script-vs-compile-function % nvm use 14
Now using node v14.18.2 (npm v6.14.15)
rishi vm-script-vs-compile-function % node index.js
Running with Script took 51 ms
Running with compileFunction took 4379 ms
Running with compileFunction and cached data took 264 ms
rishi vm-script-vs-compile-function % nvm use 16
Now using node v16.13.1 (npm v8.1.2)
rishi vm-script-vs-compile-function % node index.js
Running with Script took 4280 ms
Running with compileFunction took 4350 ms
Running with compileFunction and cached data took 321 ms
Just as a follow up here, these are the numbers I'm seeing with the latest 20.8 (nothing new, just posting to say it's still an issue).
$ nvm run 16.10 index.jsRunning node v16.10.0 (npm v7.24.0)Running with Script took 19 msRunning with compileFunction took 1799 msRunning with compileFunction and cached data took 145 ms
$ nvm run 16.11 index.jsRunning node v16.11.1 (npm v8.0.0)Running with Script took 1790 msRunning with compileFunction took 1798 msRunning with compileFunction and cached data took 147 ms
$ nvm run 20 index.jsRunning node v20.8.0 (npm v10.1.0)Running with Script took 1804 msRunning with compileFunction took 1826 msRunning with compileFunction and cached data took 141 ms
I think we can at least not have any host-defined options at all when the importModuleDynamically isn't used. That should make it possible for the cache to get hit again in that case at least.
The performance hit would comeback again if you intent to support import() though, because until https://bugs.chromium.org/p/v8/issues/detail?id=10284 gets fixed, the host defined options is going to be serialized as part of the script, and since you might still want different host-defined options for different script even if they have the same source, V8 needs to reject the cache. The fix would be to either move the host defined options to somewhere more sensible (as proposed by the V8 issue initially), or as later comments in that issue suggested, just don't serialize it and provide some kind of callback for the embedder to compute the host-defined options (personally I'd prefer that, I think that provides more flexibility).
Ooh, any workaround, even with caveats, would be awesome! 😃
The performance hit would comeback again if you intent to support import()
We do support that, but I believe that only happens if vm Modules are active (which is behind a flag)? If it's not behind a node flag, I'm happy to put it behind a Jest flag or some such so people who don't need it won't get the perf hit.
Reacted by Tolga Paksoy, Kirill Groshkov and ExE Boss
I opened #49950 to address the "no import() needed" case which is a fairly simple change and can backport to earlier release lines (v20.x at least, not 100% sure about v18.x yet). Locally the in-isolate cache is getting hit again (I used the repro from OP but switched the script being compiled to test/fixtures/snapshot/typescript.js which is also big).
❯ ./node_main bench.js
Running with Script took 5428 ms
Running with compileFunction took 5332 ms
Running with compileFunction and cached data took 1010 ms
❯ ./node_pr bench.js
Running with Script took 56 ms
Running with compileFunction took 5369 ms
Running with compileFunction and cached data took 1013 ms
Reacted by Tolga Paksoy, Christian Wiese, Bruno Farias, Kirill Groshkov and Will Slattum
Reacted by Simen Bekkhus, Christian Wiese, Bruno Farias, Kirill Groshkov, Will Slattum, Christoph Flick and Valentin Semirulnik
Reacted by Raz Luvaton, vidal7, Christian Wiese, Bruno Farias, Kirill Groshkov and Alvar Lagerlöf
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
Yes, I am still consolidating the new design but on a high level, it should stick to "one host-defined option for one loader", therefore all ESM compiled using a loader should hit the cache when compiled again using the same loader (and in the new design, the loader should be an instance of a class that stays constant for a context, not different closures that will keep changing for each compiled unit). So moving to #62720 for now.
Activity
SimenB commentedon Sep 28, 2020
I added a quick and dirty
produceCachedData, and it went down to about180 ms. Which is a great improvement, albeit still 5x slower thannew Script. And harder to manage (I'd prefer the implicit caching of thenew ScriptAPIs)devsnek commentedon Sep 28, 2020
cc @nodejs/v8, this might not be something fixable on node's side.
bnoordhuis commentedon Sep 29, 2020
This is about
v8::ScriptCompiler::CompileFunctionInContext()not using the code cache, right?I don't think that's V8's issue to fix. V8 can't know whether it's safe to use the cache because the argument names and context extensions affect how the function is parsed.
Not that it's easy for Node.js.
options.contextExtensionsis a free-form list of mutable objects. Hard to cache on that unless object identity is good enough - but it probably isn't.What Node.js (or perhaps V8) could optimize for is the presumably common case of no context extensions. TBD how to cache without introducing memory leaks.
SimenB commentedon Sep 29, 2020
In Jest's case we'll be passing a different
parsingContextall the time (Jest uses a differentContextper test file for sandboxing), so if the cached data is dependent on theContextwe'd probably still get a painful performance regression vs usingvm.Script. If it's not possible to get caching based on the source code string (and the arguments) alone, we'll probably have to drop usage ofcompileFunctionand stay onvm.Script.Using the
argsarray to vary the cache is no issue for our use case, though - 99% of the time they'll be the same, and for the cases it's not I guess it'd just create some more? Same as we have today with the manualfunction(some, args) {USER_SOURCE_CODE_HERE}wrapper we add which changes its source code via the arguments we pass in the manual function wrapper.(That said, improving performance for
compileFunctionin the case of not passingparsingContextseems like a good thing even if it wouldn't be enough for Jest's use case 🙂)bnoordhuis commentedon Sep 29, 2020
I don't think that can work so you're probably better off sticking with
vm.Script, yes. Argument names aren't insurmountable but context extensions are.When you pass in context extensions, what's evaluated looks like this in pseudo code:
I say "pseudo code" but in fact it's really close to the runtime representation.
SimenB commentedon Sep 30, 2020
@bnoordhuis reading your comment more closely, I see you're talking about
contextExtensions. We don't use that, we useparsingContext- does that have the same limitations you think?devsnek commentedon Sep 30, 2020
I think this issue should be upstreamed.
rthreei commentedon Dec 31, 2021
Fwiw, Node 16 has both Script and compileFunction performing similarly:
devsnek commentedon Dec 31, 2021
yes... in more recent versions of v8 the compilation cache is effectively broken. see v8:10284
SimenB commentedon Sep 29, 2023
Just as a follow up here, these are the numbers I'm seeing with the latest 20.8 (nothing new, just posting to say it's still an issue).
joyeecheung commentedon Sep 29, 2023
I think we can at least not have any host-defined options at all when the
importModuleDynamicallyisn't used. That should make it possible for the cache to get hit again in that case at least.The performance hit would comeback again if you intent to support
import()though, because until https://bugs.chromium.org/p/v8/issues/detail?id=10284 gets fixed, the host defined options is going to be serialized as part of the script, and since you might still want different host-defined options for different script even if they have the same source, V8 needs to reject the cache. The fix would be to either move the host defined options to somewhere more sensible (as proposed by the V8 issue initially), or as later comments in that issue suggested, just don't serialize it and provide some kind of callback for the embedder to compute the host-defined options (personally I'd prefer that, I think that provides more flexibility).SimenB commentedon Sep 29, 2023
Ooh, any workaround, even with caveats, would be awesome! 😃
We do support that, but I believe that only happens if vm Modules are active (which is behind a flag)? If it's not behind a node flag, I'm happy to put it behind a Jest flag or some such so people who don't need it won't get the perf hit.
joyeecheung commentedon Sep 29, 2023
I opened #49950 to address the "no
import()needed" case which is a fairly simple change and can backport to earlier release lines (v20.x at least, not 100% sure about v18.x yet). Locally the in-isolate cache is getting hit again (I used the repro from OP but switched the script being compiled totest/fixtures/snapshot/typescript.jswhich is also big).35 remaining items
vm: use default HDO when importModuleDynamically is not set
github-actions commentedon Jun 27, 2026
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.
fix(twenty-sdk): stop dev-mode OOM by caching compiled manifest modules
fix(twenty-sdk): stop dev-mode OOM by caching compiled manifest modules
fix(twenty-sdk): stop dev-mode OOM by caching compiled manifest modul…
SimenB commentedon Jul 8, 2026
Still relevant I would think, but possibly better tracked in #62720? Seeing as the issues have been fixed EXCEPT for when using ESM (IIRC)
joyeecheung commentedon Jul 8, 2026
Yes, I am still consolidating the new design but on a high level, it should stick to "one host-defined option for one loader", therefore all ESM compiled using a loader should hit the cache when compiled again using the same loader (and in the new design, the loader should be an instance of a class that stays constant for a context, not different closures that will keep changing for each compiled unit). So moving to #62720 for now.