Skip to content

Commit 2a84b8d

Browse files
joyeecheungjoyeecheung
committed
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.
1 parent e958ee7 commit 2a84b8d

4 files changed

Lines changed: 138 additions & 100 deletions

File tree

‎lib/internal/per_context.js‎

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,44 @@
1+
// arguments: global
2+
13
'use strict';
24

35
// node::NewContext calls this script
46

5-
(function(global) {
6-
// https://github.com/nodejs/node/issues/14909
7-
if (global.Intl) delete global.Intl.v8BreakIterator;
7+
// https://github.com/nodejs/node/issues/14909
8+
if (global.Intl) delete global.Intl.v8BreakIterator;
89

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

15-
const AtomicsNotify = global.Atomics.notify;
16-
const ReflectApply = global.Reflect.apply;
16+
const AtomicsNotify = global.Atomics.notify;
17+
const ReflectApply = global.Reflect.apply;
1718

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

21-
let wakeWarned = false;
22-
function wake(typedArray, index, count) {
23-
if (!wakeWarned) {
24-
wakeWarned = true;
22+
let wakeWarned = false;
23+
function wake(typedArray, index, count) {
24+
if (!wakeWarned) {
25+
wakeWarned = true;
2526

26-
if (global.process !== undefined) {
27-
global.process.emitWarning(warning, 'Atomics');
28-
} else {
29-
global.console.error(`Atomics: ${warning}`);
30-
}
27+
if (global.process !== undefined) {
28+
global.process.emitWarning(warning, 'Atomics');
29+
} else {
30+
global.console.error(`Atomics: ${warning}`);
3131
}
32-
33-
return ReflectApply(AtomicsNotify, this, arguments);
3432
}
3533

36-
global.Object.defineProperties(global.Atomics, {
37-
wake: {
38-
value: wake,
39-
writable: true,
40-
enumerable: false,
41-
configurable: true,
42-
},
43-
});
44-
}(this));
34+
return ReflectApply(AtomicsNotify, this, arguments);
35+
}
36+
37+
global.Object.defineProperties(global.Atomics, {
38+
wake: {
39+
value: wake,
40+
writable: true,
41+
enumerable: false,
42+
configurable: true,
43+
},
44+
});

‎src/node.cc‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,6 @@ using v8::ObjectTemplate;
162162
using v8::PropertyAttribute;
163163
using v8::ReadOnly;
164164
using v8::Script;
165-
using v8::ScriptCompiler;
166165
using v8::ScriptOrigin;
167166
using v8::SealHandleScope;
168167
using v8::SideEffectType;
@@ -2500,14 +2499,16 @@ Local<Context> NewContext(Isolate* isolate,
25002499
// Run lib/internal/per_context.js
25012500
Context::Scope context_scope(context);
25022501

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

25132514
return context;

‎src/node_native_module.cc‎

Lines changed: 59 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -103,29 +103,53 @@ void NativeModuleLoader::CompileCodeCache(
103103

104104
// TODO(joyeecheung): allow compiling cache for bootstrapper by
105105
// switching on id
106-
Local<Value> result = CompileAsModule(env, *id, true);
106+
MaybeLocal<Value> result = CompileAsModule(env, *id, true);
107107
if (!result.IsEmpty()) {
108-
args.GetReturnValue().Set(result);
108+
args.GetReturnValue().Set(result.ToLocalChecked());
109109
}
110110
}
111111

112112
void NativeModuleLoader::CompileFunction(
113113
const FunctionCallbackInfo<Value>& args) {
114114
Environment* env = Environment::GetCurrent(args);
115-
116115
CHECK(args[0]->IsString());
117116
node::Utf8Value id(env->isolate(), args[0].As<String>());
118-
Local<Value> result = CompileAsModule(env, *id, false);
117+
118+
MaybeLocal<Value> result = CompileAsModule(env, *id, false);
119119
if (!result.IsEmpty()) {
120-
args.GetReturnValue().Set(result);
120+
args.GetReturnValue().Set(result.ToLocalChecked());
121121
}
122122
}
123123

124-
Local<Value> NativeModuleLoader::CompileAsModule(Environment* env,
125-
const char* id,
126-
bool produce_code_cache) {
124+
// TODO(joyeecheung): it should be possible to generate the argument names
125+
// from some special comments for the bootstrapper case.
126+
MaybeLocal<Value> NativeModuleLoader::CompileAndCall(
127+
Local<Context> context,
128+
const char* id,
129+
std::vector<Local<String>>* parameters,
130+
std::vector<Local<Value>>* arguments,
131+
Environment* optional_env) {
132+
Isolate* isolate = context->GetIsolate();
133+
MaybeLocal<Value> compiled = per_process_loader.LookupAndCompile(
134+
context, id, parameters, false, nullptr);
135+
if (compiled.IsEmpty()) {
136+
return compiled;
137+
}
138+
Local<Function> fn = compiled.ToLocalChecked().As<Function>();
139+
return fn->Call(
140+
context, v8::Null(isolate), arguments->size(), arguments->data());
141+
}
142+
143+
MaybeLocal<Value> NativeModuleLoader::CompileAsModule(Environment* env,
144+
const char* id,
145+
bool return_code_cache) {
146+
std::vector<Local<String>> parameters = {env->exports_string(),
147+
env->require_string(),
148+
env->module_string(),
149+
env->process_string(),
150+
env->internal_binding_string()};
127151
return per_process_loader.LookupAndCompile(
128-
env->context(), id, produce_code_cache, env);
152+
env->context(), id, &parameters, return_code_cache, env);
129153
}
130154

131155
// Currently V8 only checks that the length of the source code is the
@@ -183,15 +207,18 @@ ScriptCompiler::CachedData* NativeModuleLoader::GetCachedData(
183207
return new ScriptCompiler::CachedData(code_cache_value, code_cache_length);
184208
}
185209

186-
// Returns Local<Function> of the compiled module if produce_code_cache
210+
// Returns Local<Function> of the compiled module if return_code_cache
187211
// is false (we are only compiling the function).
188212
// Otherwise return a Local<Object> containing the cache.
189-
Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
190-
const char* id,
191-
bool produce_code_cache,
192-
Environment* optional_env) {
213+
MaybeLocal<Value> NativeModuleLoader::LookupAndCompile(
214+
Local<Context> context,
215+
const char* id,
216+
std::vector<Local<String>>* parameters,
217+
bool return_code_cache,
218+
Environment* optional_env) {
193219
Isolate* isolate = context->GetIsolate();
194220
EscapableHandleScope scope(isolate);
221+
Local<Value> ret; // Used to convert to MaybeLocal before return
195222

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

@@ -209,7 +236,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
209236
// built with them.
210237
// 2. If we are generating code cache for tools/general_code_cache.js, we
211238
// are not going to use any cache ourselves.
212-
if (has_code_cache_ && !produce_code_cache) {
239+
if (has_code_cache_ && !return_code_cache) {
213240
cached_data = GetCachedData(id);
214241
if (cached_data != nullptr) {
215242
use_cache = true;
@@ -219,50 +246,33 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
219246
ScriptCompiler::Source script_source(source, origin, cached_data);
220247

221248
ScriptCompiler::CompileOptions options;
222-
if (produce_code_cache) {
249+
if (return_code_cache) {
223250
options = ScriptCompiler::kEagerCompile;
224251
} else if (use_cache) {
225252
options = ScriptCompiler::kConsumeCodeCache;
226253
} else {
227254
options = ScriptCompiler::kNoCompileOptions;
228255
}
229256

230-
MaybeLocal<Function> maybe_fun;
231-
// Currently we assume if Environment is ready, then we must be compiling
232-
// native modules instead of bootstrappers.
233-
if (optional_env != nullptr) {
234-
Local<String> parameters[] = {optional_env->exports_string(),
235-
optional_env->require_string(),
236-
optional_env->module_string(),
237-
optional_env->process_string(),
238-
optional_env->internal_binding_string()};
239-
maybe_fun = ScriptCompiler::CompileFunctionInContext(context,
240-
&script_source,
241-
arraysize(parameters),
242-
parameters,
243-
0,
244-
nullptr,
245-
options);
246-
} else {
247-
// Until we migrate bootstrappers compilations here this is unreachable
248-
// TODO(joyeecheung): it should be possible to generate the argument names
249-
// from some special comments for the bootstrapper case.
250-
// Note that for bootstrappers we may not be able to get the argument
251-
// names as env->some_string() because we might be compiling before
252-
// those strings are initialized.
253-
UNREACHABLE();
254-
}
257+
MaybeLocal<Function> maybe_fun =
258+
ScriptCompiler::CompileFunctionInContext(context,
259+
&script_source,
260+
parameters->size(),
261+
parameters->data(),
262+
0,
263+
nullptr,
264+
options);
255265

256-
Local<Function> fun;
257266
// This could fail when there are early errors in the native modules,
258267
// e.g. the syntax errors
259-
if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) {
268+
if (maybe_fun.IsEmpty()) {
260269
// In the case of early errors, v8 is already capable of
261270
// decorating the stack for us - note that we use CompileFunctionInContext
262271
// so there is no need to worry about wrappers.
263-
return scope.Escape(Local<Value>());
272+
return scope.EscapeMaybe(MaybeLocal<Value>());
264273
}
265274

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

282-
if (produce_code_cache) {
292+
if (return_code_cache) {
283293
std::unique_ptr<ScriptCompiler::CachedData> cached_data(
284294
ScriptCompiler::CreateCodeCacheForFunction(fun));
285295
CHECK_NE(cached_data, nullptr);
@@ -296,10 +306,12 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
296306
copied.release(),
297307
cached_data_length,
298308
ArrayBufferCreationMode::kInternalized);
299-
return scope.Escape(Uint8Array::New(buf, 0, cached_data_length));
309+
ret = Uint8Array::New(buf, 0, cached_data_length);
300310
} else {
301-
return scope.Escape(fun);
311+
ret = fun;
302312
}
313+
314+
return scope.EscapeMaybe(MaybeLocal<Value>(ret));
303315
}
304316

305317
void NativeModuleLoader::Initialize(Local<Object> target,

‎src/node_native_module.h‎

Lines changed: 37 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,13 @@ namespace native_module {
1616
using NativeModuleRecordMap = std::map<std::string, UnionBytes>;
1717
using NativeModuleHashMap = std::map<std::string, std::string>;
1818

19-
// The native (C++) side of the native module compilation.
20-
// This class should not depend on Environment
19+
// The native (C++) side of the NativeModule in JS land, which
20+
// handles compilation and caching of builtin modules (NativeModule)
21+
// and bootstrappers, whose source are bundled into the binary
22+
// as static data.
23+
// This class should not depend on a particular isolate, context, or
24+
// environment. Rather it should take them as arguments when necessary.
25+
// The instances of this class are per-process.
2126
class NativeModuleLoader {
2227
public:
2328
NativeModuleLoader();
@@ -26,6 +31,15 @@ class NativeModuleLoader {
2631
v8::Local<v8::Context> context);
2732
v8::Local<v8::Object> GetSourceObject(v8::Local<v8::Context> context) const;
2833
v8::Local<v8::String> GetSource(v8::Isolate* isolate, const char* id) const;
34+
// Run a script with JS source bundled inside the binary as if it's a
35+
// function called with a null receiver and arguments specified in C++.
36+
// The returned value is empty if an exception is encountered.
37+
v8::MaybeLocal<v8::Value> CompileAndCall(
38+
v8::Local<v8::Context> context,
39+
const char* id,
40+
std::vector<v8::Local<v8::String>>* parameters,
41+
std::vector<v8::Local<v8::Value>>* arguments,
42+
Environment* optional_env);
2943

3044
private:
3145
static void GetCacheUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -48,17 +62,28 @@ class NativeModuleLoader {
4862
void LoadCodeCacheHash(); // Loads data into code_cache_hash_
4963

5064
v8::ScriptCompiler::CachedData* GetCachedData(const char* id) const;
51-
static v8::Local<v8::Value> CompileAsModule(Environment* env,
52-
const char* id,
53-
bool produce_code_cache);
54-
// TODO(joyeecheung): make this public and reuse it to compile bootstrappers.
65+
66+
// Compile a script as a NativeModule that can be loaded via
67+
// NativeModule.p.require in JS land. If returns_code_cache is true,
68+
// returns the cache data in a Uint8Array, otherwise returns the compiled
69+
// function.
70+
// TODO(joyeecheung): it's possible to always produce code cache
71+
// on the main thread and consume them in worker threads, or just
72+
// share the cache among all the threads, although
73+
// We need to decide whether to do that even when workers are not used.
74+
static v8::MaybeLocal<v8::Value> CompileAsModule(Environment* env,
75+
const char* id,
76+
bool return_code_cache);
77+
5578
// For bootstrappers optional_env may be a nullptr.
56-
// This method magically knows what parameter it should pass to
57-
// the function to be compiled.
58-
v8::Local<v8::Value> LookupAndCompile(v8::Local<v8::Context> context,
59-
const char* id,
60-
bool produce_code_cache,
61-
Environment* optional_env);
79+
// If an exception is encountered (e.g. source code contains
80+
// syntax error), the returned value is empty.
81+
v8::MaybeLocal<v8::Value> LookupAndCompile(
82+
v8::Local<v8::Context> context,
83+
const char* id,
84+
std::vector<v8::Local<v8::String>>* parameters,
85+
bool returns_code_cache,
86+
Environment* optional_env);
6287

6388
bool has_code_cache_ = false;
6489
NativeModuleRecordMap source_;

0 commit comments

Comments
 (0)
⚡