vm: add createCacheForFunction #24069

ryzokuken · 2018-11-03T20:28:02Z

TODO:

Use the binding in vm.compileFunction and family, removing the C++ code that currently handles caching.

nodejs-github-bot · 2018-11-03T20:28:03Z

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1477/pipeline

src/node_contextify.cc

Remove caching logic from the CompileFunction binding from the ContextifyScript class, moving the caching logic to the JS-layer for vm.compileFunction (using the CreateCacheForFunction binding).

devsnek · 2018-11-04T20:10:14Z

lib/vm.js

    parsingContext,
    contextExtensions,
    params
  );
+
+  if (produceCachedData) {


i would rather the could just call this manually. we've learned from vm.Script that produceCachedData isn't a very good model

@devsnek that could be done. A couple of things:

That'll force us to make this an API method. vm.compileFunction used to do that, and people should be allowed to fallback to the original without breakage.

That'll be a breaking change for vm.compileFunction.

Perhaps I could do that in a separate semver-major PR? That said, I'm still not 100% convinced that this would be a good idea in the first place.

The returned values of vm.compileFunction are neither documented or tested, and it’s only been released since 10.11 (10.10?) I would say it should probably be safe to deprecate the option in 11

Also given how the returned values are used, does it actually break anyone if the cache is not generated? If we only return the function as-is, then cachedDataProduced and cachedData will always be undefined which is not that different from “the cache generation always fail”, but the users (if they exist) should always prepare to handle that case in the first place (provided they actually guess what those values are even when there are no documentations). The only exception that I can think of is our code cache generator but that’s just tooling, not any production code or anything and I don’t think this should be handled as errors in a non-tooling situation...

(The comments above are orthogonal to this PR, BTW, but if vm.compileFunction does not use it, and it’s not exposed as an API either, I couldn’t see why we are adding a binding that is not used...)

The key question is how we should handle the absence of cached data. Should it be seen as an error or a normal state? My inclination is to treat it as a normal state for non-tooling use cases, as it's unclear how extensively these values have been used in production code.

I support the idea of a separate semver-major PR to address this and consider making produceCachedData an API method. This approach provides clarity and a smoother transition for users, especially in scenarios where the use of these values is minimal.

joyeecheung · 2018-11-05T06:08:21Z

Giving this a bit more thought...maybe this should be placed under v8, as code cache is a v8 thing - conceptually for other vms this could be a noop if they don’t have an equivalent...

Trott · 2018-11-28T03:25:58Z

@ryzokuken Is this still being worked on?

bcoe · 2018-12-02T21:24:46Z

@joyeecheung @ryzokuken, echoing @Trott's curiosity; excited to get this work landed because it moves us closer to being able to simplify test coverage in Node.js.

ryzokuken · 2018-12-02T21:53:43Z

@bcoe I was out of town for a conference and other events for more than two weeks. Will jump onto these now.

jdalton · 2019-02-20T16:54:38Z

Pinging since #21573 was merged and this PR might have renewed energy.

devsnek · 2019-02-20T17:12:47Z

lib/vm.js

+    const cache = createCacheForFunction(fn);
+    fn.cachedDataProduced = cache !== undefined;
+    if (fn.cachedDataProduced) {
+      fn.cachedData = cache;


i can imagine this making people need to delete the prop after grabbing it if they want to pass the resulting function around. i'm not 100% sure what a better interface would be but i think we should brainstorm more.

This has been out since v10, though this has not been tested nor documented. We'll probably still need a deprecation cycle if we want to replace this with something else.

i think we also need to allow this to run after the function has been called. we might need to do something like vm.createCodeCacheForFunction(fn)

So @devsnek and @jdalton support exposing this as part of the vm API, unless I'm wrong? That is complete fine by me, I was actually planning of proposing that myself.

As of removing the cache production part inside vm.compileFunction, I honestly don't know. Idk if going through the deprecation cycle is worth it, but if everyone else feels it is a great idea to drop the prop, I have no real objections backed with logic.

fhinkel · 2019-10-28T23:33:17Z

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

ryzokuken · 2019-10-29T01:06:33Z

Hi @fhinkel! Thanks for the ping. @joyeecheung did you already fix this? If not, I could take a stab.

BridgeAR · 2019-12-20T01:47:22Z

Ping @joyeecheung (see question above)

jasnell · 2020-06-25T13:49:28Z

Ping @ryzokuken and @joyeecheung ... looked like this has stalled completely and needs an update.

jasnell · 2020-07-06T18:20:02Z

Closing due to lack of continued activity. Can reopen if it ends up being revisited

ryzokuken · 2020-07-06T19:14:30Z

@jasnell apologies, I haven't had a lot of time lately, but I do intend to finish this.

bnb · 2022-01-11T19:58:49Z

@ryzokuken do you still intend to finish this?

ryzokuken · 2022-01-12T11:01:06Z

@bnb sorry about that, I'll try to get back to it over the weekend.

SimenB · 2022-01-12T11:06:27Z

Does #35375 (comment) affect this?

ryzokuken · 2022-01-12T11:19:19Z

I have no clue. @devsnek wdyt?

devsnek · 2022-01-12T14:30:25Z

it's a different cache. it shouldn't stop this from landing afaik

ryzokuken · 2022-01-12T20:30:09Z

Great, then I'll start by rebasing.

fhinkel · 2024-02-03T11:10:37Z

Closing due to lack of continued activity. Can reopen if it ends up being revisited.

vm: add createCacheForFunction

f3ce13d

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Nov 3, 2018

joyeecheung reviewed Nov 4, 2018

View reviewed changes

src/node_contextify.cc Outdated Show resolved Hide resolved

ryzokuken added 3 commits November 5, 2018 00:49

fixup! vm: add createCacheForFunction

db807b5

vm: remove caching logic from CompileFunction

fc46979

Remove caching logic from the CompileFunction binding from the ContextifyScript class, moving the caching logic to the JS-layer for vm.compileFunction (using the CreateCacheForFunction binding).

fixup! vm: remove caching logic from CompileFunction

ae5534a

devsnek reviewed Nov 4, 2018

View reviewed changes

joyeecheung mentioned this pull request Nov 6, 2018

lib: modify NativeModule to use compileFunction #23837

Open

4 tasks

devsnek reviewed Feb 20, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:52

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20

jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020

jasnell closed this Jul 6, 2020

ryzokuken reopened this Jul 6, 2020

SimenB mentioned this pull request Aug 15, 2020

performance regression #26229

Closed

SimenB mentioned this pull request Sep 27, 2020

vm.compileFunction(source) is much slower than new vm.Script(source) #35375

Open

aduh95 removed the stalled Issues and PRs that are stalled. label Oct 19, 2020

aduh95 marked this pull request as draft October 19, 2020 16:47

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

devsnek marked this pull request as ready for review January 12, 2022 14:29

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

mhdawson added the test-stale-pr label May 26, 2023

fhinkel closed this Feb 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: add createCacheForFunction #24069

vm: add createCacheForFunction #24069

ryzokuken commented Nov 3, 2018

nodejs-github-bot commented Nov 3, 2018

devsnek Nov 4, 2018

ryzokuken Nov 5, 2018

joyeecheung Nov 5, 2018

joyeecheung Nov 5, 2018 •

edited

Loading

joyeecheung Nov 5, 2018

Mifrill Nov 3, 2023

joyeecheung commented Nov 5, 2018

Trott commented Nov 28, 2018

bcoe commented Dec 2, 2018

ryzokuken commented Dec 2, 2018

jdalton commented Feb 20, 2019

devsnek Feb 20, 2019 •

edited

Loading

joyeecheung Feb 20, 2019 •

edited

Loading

devsnek Feb 20, 2019

ryzokuken Feb 20, 2019

fhinkel commented Oct 28, 2019

ryzokuken commented Oct 29, 2019

BridgeAR commented Dec 20, 2019

jasnell commented Jun 25, 2020

jasnell commented Jul 6, 2020

ryzokuken commented Jul 6, 2020

bnb commented Jan 11, 2022

ryzokuken commented Jan 12, 2022

SimenB commented Jan 12, 2022

ryzokuken commented Jan 12, 2022

devsnek commented Jan 12, 2022

ryzokuken commented Jan 12, 2022

fhinkel commented Feb 3, 2024

vm: add createCacheForFunction #24069

vm: add createCacheForFunction #24069

Conversation

ryzokuken commented Nov 3, 2018

TODO:

nodejs-github-bot commented Nov 3, 2018

devsnek Nov 4, 2018

Choose a reason for hiding this comment

ryzokuken Nov 5, 2018

Choose a reason for hiding this comment

joyeecheung Nov 5, 2018

Choose a reason for hiding this comment

joyeecheung Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Nov 5, 2018

Choose a reason for hiding this comment

Mifrill Nov 3, 2023

Choose a reason for hiding this comment

joyeecheung commented Nov 5, 2018

Trott commented Nov 28, 2018

bcoe commented Dec 2, 2018

ryzokuken commented Dec 2, 2018

jdalton commented Feb 20, 2019

devsnek Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

joyeecheung Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

devsnek Feb 20, 2019

Choose a reason for hiding this comment

ryzokuken Feb 20, 2019

Choose a reason for hiding this comment

fhinkel commented Oct 28, 2019

ryzokuken commented Oct 29, 2019

BridgeAR commented Dec 20, 2019

jasnell commented Jun 25, 2020

jasnell commented Jul 6, 2020

ryzokuken commented Jul 6, 2020

bnb commented Jan 11, 2022

ryzokuken commented Jan 12, 2022

SimenB commented Jan 12, 2022

ryzokuken commented Jan 12, 2022

devsnek commented Jan 12, 2022

ryzokuken commented Jan 12, 2022

fhinkel commented Feb 3, 2024

joyeecheung Nov 5, 2018 •

edited

Loading

devsnek Feb 20, 2019 •

edited

Loading

joyeecheung Feb 20, 2019 •

edited

Loading