-
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
vm.compileFunction(source)
is much slower than new vm.Script(source)
#35375
Comments
I added a quick and dirty |
cc @nodejs/v8, this might not be something fixable on node's side. |
This is about 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. 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 Using the (That said, improving performance for |
I don't think that can work so you're probably better off sticking with 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)
function f(x) {
return x * 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 |
I think this issue should be upstreamed. |
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 |
yes... in more recent versions of v8 the compilation cache is effectively broken. see v8:10284 |
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.js
Running node v16.10.0 (npm v7.24.0)
Running with Script took 19 ms
Running with compileFunction took 1799 ms
Running with compileFunction and cached data took 145 ms
$ nvm run 16.11 index.js
Running node v16.11.1 (npm v8.0.0)
Running with Script took 1790 ms
Running with compileFunction took 1798 ms
Running with compileFunction and cached data took 147 ms
$ nvm run 20 index.js
Running node v20.8.0 (npm v10.1.0)
Running with Script took 1804 ms
Running with compileFunction took 1826 ms
Running with compileFunction and cached data took 141 ms |
I think we can at least not have any host-defined options at all when the The performance hit would comeback again if you intent to support |
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. |
I opened #49950 to address the "no
|
Amazing! |
Can confirm the PR above works - gives these results on my machine with the repo from the OP
It does seem I have to make sure to not pass in |
This is huge @joyeecheung ,thank you so much ❤️ |
So I did git bisect for v8 and the changes that cause this are v8 commit I'll see what I can do |
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: #49950 Refs: #35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
So #50137 has landed, which means now if users compile a
|
WIP in https://chromium-review.googlesource.com/c/v8/v8/+/4962094 to add in-isolate compilation cache support to
If I change Note that it still gives up on caching when context extensions are used as that can be much more complex to support. But I think context extensions are probably not used as often (we don't use it internally, at least). About parsing context - I don't think it matters in terms of caching. The caching is done on context-independent scripts (which is why the cache is per-isolate, not per-context - context here being a V8 execution context, not a context for scope resolution which is actually part of the script). Binding to the context happens much later. |
Ooh, exciting! Using |
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: nodejs#49950 Refs: nodejs#35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: #49950 Refs: #35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Looks like @joyeecheung's #49950 and #50137 will be backported to Node.js v20 in the release v20.10.0: |
This comment was marked as spam.
This comment was marked as spam.
Previously there was no isolate compilation cache support for scripts compiled Script::CompileFunction() with wrapped arguments. This patch adds support for that. Refs: nodejs/node#35375 Change-Id: Id1849961ecd1282eb2dac95829157d167a3aa9a1
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: nodejs#49950 Refs: nodejs#35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: nodejs#50137 Refs: nodejs#35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Previously there was no isolate compilation cache support for scripts compiled Script::CompileFunction() with wrapped arguments. This patch adds support for that. Refs: nodejs/node#35375 Change-Id: Id1849961ecd1282eb2dac95829157d167a3aa9a1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4962094 Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#91681}
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: #49950 Backport-PR-URL: #51004 Refs: #35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Backport-PR-URL: #51004 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Instead of using the public versions of the vm APIs internally, use the internal versions so that we can skip unnecessary argument validation. The public versions would need special care to the generation of host-defined options to hit the isolate compilation cache when imporModuleDynamically isn't used, while internally it's almost always used, so this allows us to handle the host-defined options separately. PR-URL: #50137 Backport-PR-URL: #51004 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Users cannot access any API that can be used to return a module or module namespace in this callback without --experimental-vm-modules anyway, so this would eventually lead to a rejection. This patch rejects in this case with our own error message and use a constant host-defined option for the rejection, so that scripts with the same source can still be compiled using the compilation cache if no `import()` is actually called in the script. PR-URL: #50137 Backport-PR-URL: #51004 Refs: #35375 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Original commit message: [compiler] support isolate compilation cache in CompileFunction() Previously there was no isolate compilation cache support for scripts compiled Script::CompileFunction() with wrapped arguments. This patch adds support for that. Refs: nodejs#35375 Change-Id: Id1849961ecd1282eb2dac95829157d167a3aa9a1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4962094 Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#91681} Refs: v8/v8@f4b3f6e
This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. PR-URL: nodejs#49950 Refs: nodejs#35375 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
v14.12.0
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
vm
What steps will reproduce the bug?
See https://github.com/SimenB/vm-script-vs-compile-function and run
node index.js
.It's just
yarn init -2
to fetch the bundled yarn v2 source code (to have a large JS file), then loading that intonew Script
andcompileFunction
100 times and seeing how long it takes.Running this prints the following on my machine (MBP 2018):
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 intonew 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 iterationsAdditional information
This might be the same as #26229.
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?The text was updated successfully, but these errors were encountered: