Skip to content

Commit

Permalink
src: make BuiltinLoader threadsafe and non-global
Browse files Browse the repository at this point in the history
As discussed in #45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: #45942
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
addaleax authored and RafaelGSS committed Jan 20, 2023
1 parent bbf9da8 commit e2c47cd
Show file tree
Hide file tree
Showing 15 changed files with 347 additions and 173 deletions.
13 changes: 10 additions & 3 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ MaybeLocal<Value> LoadEnvironment(
return LoadEnvironment(
env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
std::string name = "embedder_main_" + std::to_string(env->thread_id());
builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
Realm* realm = env->principal_realm();

return realm->ExecuteBootstrapper(name.c_str());
Expand Down Expand Up @@ -714,10 +714,17 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
"internal/per_context/messageport",
nullptr};

// We do not have access to a per-Environment BuiltinLoader instance
// at this point, because this code runs before an Environment exists
// in the first place. However, creating BuiltinLoader instances is
// relatively cheap and all the scripts that we may want to run at
// startup are always present in it.
thread_local builtins::BuiltinLoader builtin_loader;
for (const char** module = context_files; *module != nullptr; module++) {
Local<Value> arguments[] = {exports, primordials};
if (builtins::BuiltinLoader::CompileAndCall(
context, *module, arraysize(arguments), arguments, nullptr)
if (builtin_loader
.CompileAndCall(
context, *module, arraysize(arguments), arguments, nullptr)
.IsEmpty()) {
// Execution failed during context creation.
return Nothing<bool>();
Expand Down
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,10 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
return &destroy_async_id_list_;
}

inline builtins::BuiltinLoader* Environment::builtin_loader() {
return &builtin_loader_;
}

inline double Environment::new_async_id() {
async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];
Expand Down
9 changes: 9 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,15 @@ Environment::Environment(IsolateData* isolate_data,
thread_id_(thread_id.id == static_cast<uint64_t>(-1)
? AllocateEnvironmentThreadId().id
: thread_id.id) {
#ifdef NODE_V8_SHARED_RO_HEAP
if (!is_main_thread()) {
CHECK_NOT_NULL(isolate_data->worker_context());
// TODO(addaleax): Adjust for the embedder API snapshot support changes
builtin_loader()->CopySourceAndCodeCacheReferenceFrom(
isolate_data->worker_context()->env()->builtin_loader());
}
#endif

// We'll be creating new objects so make sure we've entered the context.
HandleScope handle_scope(isolate);

Expand Down
4 changes: 4 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,8 @@ class Environment : public MemoryRetainer {
// List of id's that have been destroyed and need the destroy() cb called.
inline std::vector<double>* destroy_async_id_list();

builtins::BuiltinLoader* builtin_loader();

std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
std::unordered_map<uint32_t, contextify::ContextifyScript*>
Expand Down Expand Up @@ -1134,6 +1136,8 @@ class Environment : public MemoryRetainer {

std::unique_ptr<Realm> principal_realm_ = nullptr;

builtins::BuiltinLoader builtin_loader_;

// Used by allocate_managed_buffer() and release_managed_buffer() to keep
// track of the BackingStore for a given pointer.
std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>
Expand Down
5 changes: 0 additions & 5 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,6 @@

namespace node {

using builtins::BuiltinLoader;

using v8::EscapableHandleScope;
using v8::Isolate;
using v8::Local;
Expand Down Expand Up @@ -1183,9 +1181,6 @@ ExitCode LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
}
}

if ((*snapshot_data_ptr) != nullptr) {
BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
}
NodeMainInstance main_instance(*snapshot_data_ptr,
uv_default_loop(),
per_process::v8_platform.Platform(),
Expand Down
4 changes: 2 additions & 2 deletions src/node_binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,13 +631,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
DefineConstants(isolate, exports);
} else if (!strcmp(*module_v, "natives")) {
exports = builtins::BuiltinLoader::GetSourceObject(context);
exports = realm->env()->builtin_loader()->GetSourceObject(context);
// Legacy feature: process.binding('natives').config contains stringified
// config.gypi
CHECK(exports
->Set(context,
realm->isolate_data()->config_string(),
builtins::BuiltinLoader::GetConfigString(isolate))
realm->env()->builtin_loader()->GetConfigString(isolate))
.FromJust());
} else {
return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);
Expand Down
Loading

0 comments on commit e2c47cd

Please sign in to comment.