GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: use NativeModuleLoader to compile per_context.js by joyeecheung · Pull Request #24660 · nodejs/node

Conversation

joyeecheung commented Nov 26, 2018

Copy link

Copy Markdown

Member

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under lib/ as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the return
value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction


          src: use NativeModuleLoader to compile per_context.js

2a84b8d

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under `lib/` as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the return
value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

nodejs-github-bot commented Nov 26, 2018

Copy link

Copy Markdown

Collaborator

@joyeecheung sadly an error occured when I tried to trigger a build :(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label

Nov 26, 2018

joyeecheung commented Nov 26, 2018

Copy link

Copy Markdown

Member Author

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

cc @devsnek @addaleax

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack approved these changes

Nov 26, 2018

View reviewed changes

refack added the module Issues and PRs related to the module subsystem. label

Nov 26, 2018

devsnek approved these changes

Nov 26, 2018

View reviewed changes

Comment thread

lib/internal/per_context.js

		@@ -1,44 +1,44 @@
		// arguments: global

devsnek Nov 26, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the important part here wasn't the global, it was creating the closure to avoid variables leaking, since this is a script. can you make a note of that here or in node.cc so we don't accidentally break things in the future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Nov 26, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek I thought about that, but how does one break this from the JS side? This source is now wrapped inside a function provided by CompileFunctionInContext, so any declaration in this file goes into the scope of that function. Since we are in strict mode, one cannot accidentally declare a global, and as you can see here, the global object is...global. Also, additional properties should be caught by leakedGlobals in the test/common unless someone deliberately add them via Object.defineProperty with additional attributes..

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

devsnek Nov 26, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung i'm not saying the current code doesn't work, just that there should be a note mentioning that this code expects a closure of some sort. if it were changed in the future, we could end up leaking the variables like AtomicsNotify to all evaluations in node.js

> vm.runInThisContext('const a = 5')
undefined
> vm.runInThisContext('a')
5
>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Nov 27, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek: I see...but I think that can be said about all the built in JS, this just puts the per_context.js in the same situation as other native modules, maybe we should find a better place to document about them all...(I have a WIP that migrates the other two bootstrapper, once we are done they are all in the same situation)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Nov 27, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in node_native_module.h? Or even a README in lib?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis reviewed

Nov 27, 2018

View reviewed changes

Comment thread

src/node_native_module.cc Outdated

		// decorating the stack for us - note that we use CompileFunctionInContext
		// so there is no need to worry about wrappers.
		return scope.Escape(Local<Value>());
		return scope.EscapeMaybe(MaybeLocal<Value>());

bnoordhuis Nov 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return MaybeLocal<Value>(); should work, no need to go through EscapeMaybe() first.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_native_module.cc Outdated

		ret = fun;
		}

		return scope.EscapeMaybe(MaybeLocal<Value>(ret));

bnoordhuis Nov 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return scope.Escape(ret);?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node_native_module.h Outdated

		// We need to decide whether to do that even when workers are not used.
		static v8::MaybeLocal<v8::Value> CompileAsModule(Environment* env,
		const char* id,
		bool return_code_cache);

bnoordhuis Nov 27, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you didn't introduce it but the bool would be clearer at call sites as an enum.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

joyeecheung Nov 27, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea (In fact, it was me who introduced this bool, just not in this patch)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

WillTheSun reacted with laugh emoji

All reactions

😄 1 reaction


          fixup! src: use NativeModuleLoader to compile per_context.js

9151e7a

joyeecheung force-pushed the per-context-native branch from 6cc42f8 to 9151e7a Compare

November 27, 2018 15:23

joyeecheung commented Nov 27, 2018

Copy link

Copy Markdown

Member Author

@devsnek @bnoordhuis Thanks for the reviews, updated.

I added a few more comments in node_native_module.h, not sure if that's enough to warn people that the safety of closure is provided from the C++ layer, but I guess we could naturally adapt since that's what we already assume about requireable native modules anyway. This just makes the same guarantee for bootstrappers by using the same mechanism to compile them.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung commented Nov 28, 2018

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Nov 28, 2018

Trott pushed a commit to Trott/io.js that referenced this pull request

Nov 28, 2018


          src: use NativeModuleLoader to compile per_context.js

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under `lib/` as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the
return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

PR-URL: nodejs#24660
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>

Trott commented Nov 28, 2018

Copy link

Copy Markdown

Member

Landed in 8041380.

Thank you for using --fixup. 😄

joyeecheung reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Trott closed this

Nov 28, 2018

targos pushed a commit that referenced this pull request

Nov 29, 2018


          src: use NativeModuleLoader to compile per_context.js

c708abb

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under `lib/` as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the
return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

PR-URL: #24660
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>

BridgeAR mentioned this pull request

Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks done

This was referenced Dec 7, 2018

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264

Closed

2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265

Closed

refack pushed a commit to refack/node that referenced this pull request

Jan 14, 2019


          src: use NativeModuleLoader to compile per_context.js

b9a97c2

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under `lib/` as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the
return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

PR-URL: nodejs#24660
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>

BethGriggs added the backport-requested-v10.x label

Feb 12, 2019

BethGriggs commented Feb 12, 2019

Copy link

Copy Markdown

Member

@joyeecheung this change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

bnoordhuis

bnoordhuis left review comments

+2 more reviewers

devsnek

devsnek approved these changes

refack

refack approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

module

Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

joyeecheung commented Nov 26, 2018

Checklist

Uh oh!

nodejs-github-bot commented Nov 26, 2018

Uh oh!

joyeecheung commented Nov 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 27, 2018

Uh oh!

joyeecheung commented Nov 28, 2018

Uh oh!

Trott commented Nov 28, 2018

Uh oh!

BethGriggs commented Feb 12, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joyeecheung Nov 26, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.