Skip to content
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.

Sign up for GitHub

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

Jump to bottom

src: use NativeModuleLoader to compile per_context.js #24660

Closed
wants to merge 2 commits into from
Closed

src: use NativeModuleLoader to compile per_context.js #24660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a84b8d
src: use NativeModuleLoader to compile per_context.js
joyeecheung Nov 26, 2018
9151e7a
fixup! src: use NativeModuleLoader to compile per_context.js
joyeecheung Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 32 additions & 32 deletions lib/internal/per_context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,44 @@
// arguments: global
Copy link
Member

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?

Copy link
Member Author

@joyeecheung joyeecheung Nov 26, 2018 •
edited
Loading

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..

Copy link
Member

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
>

Copy link
Member Author

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)

Copy link
Member Author

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?


'use strict';

// node::NewContext calls this script

(function(global) {
// https://github.com/nodejs/node/issues/14909
if (global.Intl) delete global.Intl.v8BreakIterator;
// https://github.com/nodejs/node/issues/14909
if (global.Intl) delete global.Intl.v8BreakIterator;

// https://github.com/nodejs/node/issues/21219
// Adds Atomics.notify and warns on first usage of Atomics.wake
// https://github.com/v8/v8/commit/c79206b363 adds Atomics.notify so
// now we alias Atomics.wake to notify so that we can remove it
// semver major without worrying about V8.
// https://github.com/nodejs/node/issues/21219
// Adds Atomics.notify and warns on first usage of Atomics.wake
// https://github.com/v8/v8/commit/c79206b363 adds Atomics.notify so
// now we alias Atomics.wake to notify so that we can remove it
// semver major without worrying about V8.

const AtomicsNotify = global.Atomics.notify;
const ReflectApply = global.Reflect.apply;
const AtomicsNotify = global.Atomics.notify;
const ReflectApply = global.Reflect.apply;

const warning = 'Atomics.wake will be removed in a future version, ' +
'use Atomics.notify instead.';
const warning = 'Atomics.wake will be removed in a future version, ' +
'use Atomics.notify instead.';

let wakeWarned = false;
function wake(typedArray, index, count) {
if (!wakeWarned) {
wakeWarned = true;
let wakeWarned = false;
function wake(typedArray, index, count) {
if (!wakeWarned) {
wakeWarned = true;

if (global.process !== undefined) {
global.process.emitWarning(warning, 'Atomics');
} else {
global.console.error(`Atomics: ${warning}`);
}
if (global.process !== undefined) {
global.process.emitWarning(warning, 'Atomics');
} else {
global.console.error(`Atomics: ${warning}`);
}

return ReflectApply(AtomicsNotify, this, arguments);
}

global.Object.defineProperties(global.Atomics, {
wake: {
value: wake,
writable: true,
enumerable: false,
configurable: true,
},
});
}(this));
return ReflectApply(AtomicsNotify, this, arguments);
}

global.Object.defineProperties(global.Atomics, {
wake: {
value: wake,
writable: true,
enumerable: false,
configurable: true,
},
});
19 changes: 10 additions & 9 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ using v8::ObjectTemplate;
using v8::PropertyAttribute;
using v8::ReadOnly;
using v8::Script;
using v8::ScriptCompiler;
using v8::ScriptOrigin;
using v8::SealHandleScope;
using v8::SideEffectType;
Expand Down Expand Up @@ -2500,14 +2499,16 @@ Local<Context> NewContext(Isolate* isolate,
// Run lib/internal/per_context.js
Context::Scope context_scope(context);

// TODO(joyeecheung): use NativeModuleLoader::Compile
Local<String> per_context =
per_process_loader.GetSource(isolate, "internal/per_context");
ScriptCompiler::Source per_context_src(per_context, nullptr);
Local<Script> s = ScriptCompiler::Compile(
context,
&per_context_src).ToLocalChecked();
s->Run(context).ToLocalChecked();
std::vector<Local<String>> parameters = {
FIXED_ONE_BYTE_STRING(isolate, "global")};
std::vector<Local<Value>> arguments = {context->Global()};
MaybeLocal<Value> result = per_process_loader.CompileAndCall(
context, "internal/per_context", &parameters, &arguments, nullptr);
if (result.IsEmpty()) {
// Execution failed during context creation.
// TODO(joyeecheung): deprecate this signature and return a MaybeLocal.
return Local<Context>();
}
}

return context;
Expand Down
107 changes: 60 additions & 47 deletions src/node_native_module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,29 +103,54 @@ void NativeModuleLoader::CompileCodeCache(

// TODO(joyeecheung): allow compiling cache for bootstrapper by
// switching on id
Local<Value> result = CompileAsModule(env, *id, true);
MaybeLocal<Value> result =
CompileAsModule(env, *id, CompilationResultType::kCodeCache);
if (!result.IsEmpty()) {
args.GetReturnValue().Set(result);
args.GetReturnValue().Set(result.ToLocalChecked());
}
}

void NativeModuleLoader::CompileFunction(
const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsString());
node::Utf8Value id(env->isolate(), args[0].As<String>());
Local<Value> result = CompileAsModule(env, *id, false);

MaybeLocal<Value> result =
CompileAsModule(env, *id, CompilationResultType::kFunction);
if (!result.IsEmpty()) {
args.GetReturnValue().Set(result);
args.GetReturnValue().Set(result.ToLocalChecked());
}
}

Local<Value> NativeModuleLoader::CompileAsModule(Environment* env,
const char* id,
bool produce_code_cache) {
// TODO(joyeecheung): it should be possible to generate the argument names
// from some special comments for the bootstrapper case.
MaybeLocal<Value> NativeModuleLoader::CompileAndCall(
Local<Context> context,
const char* id,
std::vector<Local<String>>* parameters,
std::vector<Local<Value>>* arguments,
Environment* optional_env) {
Isolate* isolate = context->GetIsolate();
MaybeLocal<Value> compiled = per_process_loader.LookupAndCompile(
context, id, parameters, CompilationResultType::kFunction, nullptr);
if (compiled.IsEmpty()) {
return compiled;
}
Local<Function> fn = compiled.ToLocalChecked().As<Function>();
return fn->Call(
context, v8::Null(isolate), arguments->size(), arguments->data());
}

MaybeLocal<Value> NativeModuleLoader::CompileAsModule(
Environment* env, const char* id, CompilationResultType result) {
std::vector<Local<String>> parameters = {env->exports_string(),
env->require_string(),
env->module_string(),
env->process_string(),
env->internal_binding_string()};
return per_process_loader.LookupAndCompile(
env->context(), id, produce_code_cache, env);
env->context(), id, &parameters, result, env);
}

// Currently V8 only checks that the length of the source code is the
Expand Down Expand Up @@ -183,15 +208,18 @@ ScriptCompiler::CachedData* NativeModuleLoader::GetCachedData(
return new ScriptCompiler::CachedData(code_cache_value, code_cache_length);
}

// Returns Local<Function> of the compiled module if produce_code_cache
// Returns Local<Function> of the compiled module if return_code_cache
// is false (we are only compiling the function).
// Otherwise return a Local<Object> containing the cache.
Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
const char* id,
bool produce_code_cache,
Environment* optional_env) {
MaybeLocal<Value> NativeModuleLoader::LookupAndCompile(
Local<Context> context,
const char* id,
std::vector<Local<String>>* parameters,
CompilationResultType result_type,
Environment* optional_env) {
Isolate* isolate = context->GetIsolate();
EscapableHandleScope scope(isolate);
Local<Value> ret; // Used to convert to MaybeLocal before return

Local<String> source = GetSource(isolate, id);

Expand All @@ -209,7 +237,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
// built with them.
// 2. If we are generating code cache for tools/general_code_cache.js, we
// are not going to use any cache ourselves.
if (has_code_cache_ && !produce_code_cache) {
if (has_code_cache_ && result_type == CompilationResultType::kFunction) {
cached_data = GetCachedData(id);
if (cached_data != nullptr) {
use_cache = true;
Expand All @@ -219,50 +247,33 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
ScriptCompiler::Source script_source(source, origin, cached_data);

ScriptCompiler::CompileOptions options;
if (produce_code_cache) {
if (result_type == CompilationResultType::kCodeCache) {
options = ScriptCompiler::kEagerCompile;
} else if (use_cache) {
options = ScriptCompiler::kConsumeCodeCache;
} else {
options = ScriptCompiler::kNoCompileOptions;
}

MaybeLocal<Function> maybe_fun;
// Currently we assume if Environment is ready, then we must be compiling
// native modules instead of bootstrappers.
if (optional_env != nullptr) {
Local<String> parameters[] = {optional_env->exports_string(),
optional_env->require_string(),
optional_env->module_string(),
optional_env->process_string(),
optional_env->internal_binding_string()};
maybe_fun = ScriptCompiler::CompileFunctionInContext(context,
&script_source,
arraysize(parameters),
parameters,
0,
nullptr,
options);
} else {
// Until we migrate bootstrappers compilations here this is unreachable
// TODO(joyeecheung): it should be possible to generate the argument names
// from some special comments for the bootstrapper case.
// Note that for bootstrappers we may not be able to get the argument
// names as env->some_string() because we might be compiling before
// those strings are initialized.
UNREACHABLE();
}
MaybeLocal<Function> maybe_fun =
ScriptCompiler::CompileFunctionInContext(context,
&script_source,
parameters->size(),
parameters->data(),
0,
nullptr,
options);

Local<Function> fun;
// This could fail when there are early errors in the native modules,
// e.g. the syntax errors
if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) {
if (maybe_fun.IsEmpty()) {
// In the case of early errors, v8 is already capable of
// 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 MaybeLocal<Value>();
}

Local<Function> fun = maybe_fun.ToLocalChecked();
if (use_cache) {
if (optional_env != nullptr) {
// This could happen when Node is run with any v8 flag, but
Expand All @@ -279,7 +290,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
}
}

if (produce_code_cache) {
if (result_type == CompilationResultType::kCodeCache) {
std::unique_ptr<ScriptCompiler::CachedData> cached_data(
ScriptCompiler::CreateCodeCacheForFunction(fun));
CHECK_NE(cached_data, nullptr);
Expand All @@ -296,10 +307,12 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
copied.release(),
cached_data_length,
ArrayBufferCreationMode::kInternalized);
return scope.Escape(Uint8Array::New(buf, 0, cached_data_length));
ret = Uint8Array::New(buf, 0, cached_data_length);
} else {
return scope.Escape(fun);
ret = fun;
}

return scope.Escape(ret);
}

void NativeModuleLoader::Initialize(Local<Object> target,
Expand Down
54 changes: 42 additions & 12 deletions src/node_native_module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,43 @@ namespace native_module {
using NativeModuleRecordMap = std::map<std::string, UnionBytes>;
using NativeModuleHashMap = std::map<std::string, std::string>;

// The native (C++) side of the native module compilation.
// This class should not depend on Environment
// The native (C++) side of the NativeModule in JS land, which
// handles compilation and caching of builtin modules (NativeModule)
// and bootstrappers, whose source are bundled into the binary
// as static data.
// This class should not depend on a particular isolate, context, or
// environment. Rather it should take them as arguments when necessary.
// The instances of this class are per-process.
class NativeModuleLoader {
public:
// kCodeCache indicates that the compilation result should be returned
// as a Uint8Array, whereas kFunction indicates that the result should
// be returned as a Function.
// TODO(joyeecheung): it's possible to always produce code cache
// on the main thread and consume them in worker threads, or just
// share the cache among all the threads, although
// we need to decide whether to do that even when workers are not used.
enum class CompilationResultType { kCodeCache, kFunction };

NativeModuleLoader();
static void Initialize(v8::Local<v8::Object> target,
v8::Local<v8::Value> unused,
v8::Local<v8::Context> context);
v8::Local<v8::Object> GetSourceObject(v8::Local<v8::Context> context) const;
v8::Local<v8::String> GetSource(v8::Isolate* isolate, const char* id) const;

// Run a script with JS source bundled inside the binary as if it's wrapped
// in a function called with a null receiver and arguments specified in C++.
// The returned value is empty if an exception is encountered.
// JS code run with this method can assume that their top-level
// declarations won't affect the global scope.
v8::MaybeLocal<v8::Value> CompileAndCall(
v8::Local<v8::Context> context,
const char* id,
std::vector<v8::Local<v8::String>>* parameters,
std::vector<v8::Local<v8::Value>>* arguments,
Environment* optional_env);

private:
static void GetCacheUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
// For legacy process.binding('natives') which is mutable, and for
Expand All @@ -48,17 +74,21 @@ class NativeModuleLoader {
void LoadCodeCacheHash(); // Loads data into code_cache_hash_

v8::ScriptCompiler::CachedData* GetCachedData(const char* id) const;
static v8::Local<v8::Value> CompileAsModule(Environment* env,
const char* id,
bool produce_code_cache);
// TODO(joyeecheung): make this public and reuse it to compile bootstrappers.

// Compile a script as a NativeModule that can be loaded via
// NativeModule.p.require in JS land.
static v8::MaybeLocal<v8::Value> CompileAsModule(
Environment* env, const char* id, CompilationResultType result_type);

// For bootstrappers optional_env may be a nullptr.
// This method magically knows what parameter it should pass to
// the function to be compiled.
v8::Local<v8::Value> LookupAndCompile(v8::Local<v8::Context> context,
const char* id,
bool produce_code_cache,
Environment* optional_env);
// If an exception is encountered (e.g. source code contains
// syntax error), the returned value is empty.
v8::MaybeLocal<v8::Value> LookupAndCompile(
v8::Local<v8::Context> context,
const char* id,
std::vector<v8::Local<v8::String>>* parameters,
CompilationResultType result_type,
Environment* optional_env);

bool has_code_cache_ = false;
NativeModuleRecordMap source_;
Expand Down