lib: modify NativeModule to use compileFunction #23837

ryzokuken · 2018-10-23T18:45:09Z

Modify the NativeModule class to use compileFunction instead of manually
wrapping the string source code and then compiling it.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

/cc @devsnek @bcoe

Modify the NativeModule class to use compileFunction instead of manually wrapping the string source code and then compiling it.

nodejs-github-bot · 2018-10-23T18:45:12Z

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

bcoe · 2018-10-23T19:03:22Z

@ryzokuken awesome, thank you for doing this (I'm excited to see if it fixes some of the issues I was seeing around instrumenting internal modules for coverage).

I will get some feedback to you tonight.

devsnek · 2018-10-23T20:22:49Z

lib/internal/bootstrap/loaders.js

-      const script = new ContextifyScript(
-        source, this.filename, 0, 0,
-        cache, false, undefined
+      const fn = internalBinding('contextify').compileFunction(


is there any reason to call internalBinding('contextify') every time?

Umm, it's only called once? Had it been called multiple times, I would have refactored it into a single destructure statement up top.

its called every time we load an internal module, and we have a great many of those.

@devsnek Oh wait, I get it now, sorry. Will change this.

lib/internal/bootstrap/loaders.js

bcoe

this seems to do the trick, and remove the necessity to take intro account wrapper length when calculating coverage (confirmed here).

It doesn't (as I'd hoped it might) solve the issue described here #22919 (comment)

jdalton · 2018-10-24T03:35:27Z

lib/internal/bootstrap/loaders.js

+        cache,
+        false,
+        undefined,
+        [],


☝️ I think contextExtensions can be undefined too.

@jdalton I used an empty array since that's the default value I used in vm.compileFunction and wanted to make this as consistent as I could. I just rechecked the code to make sure, the two should be functionally similar, resulting in an empty std::vector<Local<Object>> context_extensions.

My point was more reducing throw away object creation.

Add functionality in vm.compileFunction/CompileFunction to report if cache data was provided yet rejected by v8.

ryzokuken · 2018-10-24T12:26:18Z

@devsnek done! PTAL.

lib/internal/bootstrap/loaders.js

ryzokuken · 2018-10-25T00:23:17Z

@joyeecheung @jdalton done! PTAL.

src/node_contextify.cc

lib/internal/bootstrap/loaders.js

joyeecheung

Made a suggestion about commenting on the params. Let's try GitHub beta magic!

lib/internal/bootstrap/loaders.js

ryzokuken · 2018-10-27T14:48:23Z

@joyeecheung done! PTAL.

lib/internal/bootstrap/loaders.js

ryzokuken · 2018-10-30T09:35:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18227/

joyeecheung · 2018-10-30T14:29:14Z

src/node_contextify.cc

@@ -1099,12 +1099,23 @@ void ContextifyContext::CompileFunction(
          env->cached_data_string(),
          buf.ToLocalChecked()).IsNothing()) return;
    }
+
+    // Report if cache was produced.


I looked into the implementation a bit, and I wonder if we can just wrap ScriptCompiler::CreateCodeCacheForFunction in another binding that takes a function as argument? That way we can just manipulate the return values in the JS land, and also enable the code cache generator to use the exact function compiled to generate the cache if we simply return fn in NativeModule.prototype.compile - also, we don't actually need that argument handling complexity in the compileFunction C++ binding, vm.compileFunction can handle produce_cached_data in JS land and NativeModule.prototype.compile doesn't even need it at all.

Also I just noticed that the docs of vm.compileFunction did not mention its return value. Is that intentional?

(and the returned values are not tested either?)

Trott · 2018-11-06T05:18:26Z

@ryzokuken Is this still being worked on? (If yes, should we add an in progress label? If not, should we close it?)

ryzokuken · 2018-11-06T06:15:17Z

@Trott it is! The discussion was moved into another sub-change which should be merged soon and unblock this one.

joyeecheung · 2018-11-06T06:26:46Z

Pointer: #24069

Until that lands, or gets merged into this PR, it will fail the cache generation tests. Although even with that change, this still needs to fix the generator..

bcoe · 2019-01-19T00:51:14Z

@joyeecheung @ryzokuken I believe internal modules no longer have a wrapper prefix? is this work still needed?

Seems like the important issue is #21573

ryzokuken · 2019-01-19T14:19:33Z

@bcoe IIRC, @joyeecheung took care of this. Working on merging the other one in.

Trott · 2020-06-23T22:36:42Z

This looks like it may have been abandoned. Should we close this? We can re-open it if someone starts to work on it again?

ryzokuken · 2020-06-28T08:17:46Z

@Trott I had a talk with @joyeecheung about these, and looks like they still need some work. Please leave it open and I'd try to look deeper into it either later today or next week.

gireeshpunathil · 2020-12-01T11:06:39Z

ping @ryzokuken to see if it is moving

ryzokuken · 2020-12-01T11:19:41Z

I'm super sorry, but I've had no time as of late to finish this. If you don't mind leaving this open, I can try to look into it during vacations later this month or someone else would be interested to pick it up?

bnb · 2022-01-07T23:23:11Z

@ryzokuken any chance you're able to get to this?

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

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

aduh95 · 2024-05-11T16:54:56Z

This needs a rebase.

lib: modify NativeModule to use compileFunction

dc9f7e1

Modify the NativeModule class to use compileFunction instead of manually wrapping the string source code and then compiling it.

devsnek reviewed Oct 23, 2018

View reviewed changes

lib/internal/bootstrap/loaders.js Show resolved Hide resolved

devsnek added the vm Issues and PRs related to the vm subsystem. label Oct 23, 2018

fixup! lib: modify NativeModule to use compileFunction

efc0892

bcoe approved these changes Oct 24, 2018

View reviewed changes

jdalton reviewed Oct 24, 2018

View reviewed changes

ryzokuken added 2 commits October 24, 2018 17:52

vm: report cache rejection in vm.compileFunction

19262a0

Add functionality in vm.compileFunction/CompileFunction to report if cache data was provided yet rejected by v8.

fixup! lib: modify NativeModule to use compileFunction

9ec01a2

joyeecheung reviewed Oct 24, 2018

View reviewed changes

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/loaders.js Show resolved Hide resolved

fixup! lib: modify NativeModule to use compileFunction

382e3e8

jdalton reviewed Oct 25, 2018

View reviewed changes

src/node_contextify.cc Show resolved Hide resolved

joyeecheung reviewed Oct 25, 2018

View reviewed changes

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

fixup! lib: modify NativeModule to use compileFunction

72d542b

joyeecheung reviewed Oct 25, 2018

View reviewed changes

lib/internal/bootstrap/loaders.js Show resolved Hide resolved

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

fixup! lib: modify NativeModule to use compileFunction

690fe8b

ryzokuken mentioned this pull request Oct 27, 2018

vm.compileFunction should return an object #23923

Closed

fixup! lib: modify NativeModule to use compileFunction

ab4d9ad

bcoe mentioned this pull request Oct 28, 2018

coverage: NODE_V8_COVERAGE for Node's test suite, remaining blockers #22919

Closed

7 tasks

joyeecheung reviewed Oct 29, 2018

View reviewed changes

lib/internal/bootstrap/loaders.js Outdated Show resolved Hide resolved

fixup! lib: modify NativeModule to use compileFunction

d2dcd54

jasnell approved these changes Oct 30, 2018

View reviewed changes

ryzokuken added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 30, 2018

joyeecheung reviewed Oct 30, 2018

View reviewed changes

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 2, 2018

Trott added the blocked PRs that are blocked by other issues or PRs. label Nov 6, 2018

joyeecheung mentioned this pull request Nov 7, 2018

src: compile native modules and their code cache in C++ #24221

Closed

3 tasks

bcoe mentioned this pull request Dec 2, 2018

module: use compileFunction over Module.wrap #21573

Closed

4 tasks

bcoe mentioned this pull request Dec 15, 2018

Coverage reporting is broken #24966

Closed

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: modify NativeModule to use compileFunction #23837

lib: modify NativeModule to use compileFunction #23837

ryzokuken commented Oct 23, 2018 •

edited

Loading

nodejs-github-bot commented Oct 23, 2018

bcoe commented Oct 23, 2018

devsnek Oct 23, 2018 •

edited

Loading

ryzokuken Oct 23, 2018

devsnek Oct 23, 2018

ryzokuken Oct 23, 2018

bcoe left a comment

jdalton Oct 24, 2018

ryzokuken Oct 24, 2018

jdalton Oct 24, 2018

ryzokuken commented Oct 24, 2018

ryzokuken commented Oct 25, 2018

joyeecheung left a comment

ryzokuken commented Oct 27, 2018

ryzokuken commented Oct 30, 2018

joyeecheung Oct 30, 2018

joyeecheung Oct 30, 2018

joyeecheung Oct 30, 2018

Trott commented Nov 6, 2018

ryzokuken commented Nov 6, 2018

joyeecheung commented Nov 6, 2018

bcoe commented Jan 19, 2019

ryzokuken commented Jan 19, 2019

Trott commented Jun 23, 2020

ryzokuken commented Jun 28, 2020

gireeshpunathil commented Dec 1, 2020

ryzokuken commented Dec 1, 2020

bnb commented Jan 7, 2022

ryzokuken commented Jan 12, 2022

aduh95 commented May 11, 2024

lib: modify NativeModule to use compileFunction #23837

Are you sure you want to change the base?

lib: modify NativeModule to use compileFunction #23837

Conversation

ryzokuken commented Oct 23, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 23, 2018

bcoe commented Oct 23, 2018

devsnek Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Oct 24, 2018

ryzokuken commented Oct 25, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

ryzokuken commented Oct 27, 2018

ryzokuken commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 6, 2018

ryzokuken commented Nov 6, 2018

joyeecheung commented Nov 6, 2018

bcoe commented Jan 19, 2019

ryzokuken commented Jan 19, 2019

Trott commented Jun 23, 2020

ryzokuken commented Jun 28, 2020

gireeshpunathil commented Dec 1, 2020

ryzokuken commented Dec 1, 2020

bnb commented Jan 7, 2022

ryzokuken commented Jan 12, 2022

aduh95 commented May 11, 2024

ryzokuken commented Oct 23, 2018 •

edited

Loading

devsnek Oct 23, 2018 •

edited

Loading