FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: make BuiltinLoader threadsafe and non-global · nodejs/node@3551a19 · GitHub

/ node Public

Commit 3551a19

Browse files
addaleax
authored and
targos
committed
src: make BuiltinLoader threadsafe and non-global
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>
1 parent 92311a0 commit 3551a19

14 files changed

Lines changed: 293 additions & 173 deletions

‎src/api/environment.cc‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ MaybeLocal<Value> LoadEnvironment(
489489
return LoadEnvironment(
490490
env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
491491
std::string name = "embedder_main_" + std::to_string(env->thread_id());
492-
builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
492+
env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
493493
Realm* realm = env->principal_realm();
494494

495495
return realm->ExecuteBootstrapper(name.c_str());
@@ -729,10 +729,17 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
729729
"internal/per_context/messageport",
730730
nullptr};
731731

732+
// We do not have access to a per-Environment BuiltinLoader instance
733+
// at this point, because this code runs before an Environment exists
734+
// in the first place. However, creating BuiltinLoader instances is
735+
// relatively cheap and all the scripts that we may want to run at
736+
// startup are always present in it.
737+
thread_local builtins::BuiltinLoader builtin_loader;
732738
for (const char** module = context_files; *module != nullptr; module++) {
733739
Local<Value> arguments[] = {exports, primordials};
734-
if (builtins::BuiltinLoader::CompileAndCall(
735-
context, *module, arraysize(arguments), arguments, nullptr)
740+
if (builtin_loader
741+
.CompileAndCall(
742+
context, *module, arraysize(arguments), arguments, nullptr)
736743
.IsEmpty()) {
737744
// Execution failed during context creation.
738745
return Nothing<bool>();

‎src/env-inl.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,10 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
388388
return &destroy_async_id_list_;
389389
}
390390

391+
inline builtins::BuiltinLoader* Environment::builtin_loader() {
392+
return &builtin_loader_;
393+
}
394+
391395
inline double Environment::new_async_id() {
392396
async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
393397
return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];

‎src/env.cc‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -676,6 +676,15 @@ Environment::Environment(IsolateData* isolate_data,
676676
thread_id_(thread_id.id == static_cast<uint64_t>(-1)
677677
? AllocateEnvironmentThreadId().id
678678
: thread_id.id) {
679+
#ifdef NODE_V8_SHARED_RO_HEAP
680+
if (!is_main_thread()) {
681+
CHECK_NOT_NULL(isolate_data->worker_context());
682+
// TODO(addaleax): Adjust for the embedder API snapshot support changes
683+
builtin_loader()->CopySourceAndCodeCacheReferenceFrom(
684+
isolate_data->worker_context()->env()->builtin_loader());
685+
}
686+
#endif
687+
679688
// We'll be creating new objects so make sure we've entered the context.
680689
HandleScope handle_scope(isolate);
681690

‎src/env.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,8 @@ class Environment : public MemoryRetainer {
695695
// List of id's that have been destroyed and need the destroy() cb called.
696696
inline std::vector<double>* destroy_async_id_list();
697697

698+
builtins::BuiltinLoader* builtin_loader();
699+
698700
std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
699701
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
700702
std::unordered_map<uint32_t, contextify::ContextifyScript*>
@@ -1099,6 +1101,8 @@ class Environment : public MemoryRetainer {
10991101

11001102
std::unique_ptr<Realm> principal_realm_ = nullptr;
11011103

1104+
builtins::BuiltinLoader builtin_loader_;
1105+
11021106
// Used by allocate_managed_buffer() and release_managed_buffer() to keep
11031107
// track of the BackingStore for a given pointer.
11041108
std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>

‎src/node.cc‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,6 @@
132132

133133
namespace node {
134134

135-
using builtins::BuiltinLoader;
136-
137135
using v8::EscapableHandleScope;
138136
using v8::Isolate;
139137
using v8::Local;
@@ -1214,9 +1212,6 @@ int LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
12141212
}
12151213
}
12161214

1217-
if ((*snapshot_data_ptr) != nullptr) {
1218-
BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
1219-
}
12201215
NodeMainInstance main_instance(*snapshot_data_ptr,
12211216
uv_default_loop(),
12221217
per_process::v8_platform.Platform(),

‎src/node_binding.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -650,13 +650,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
650650
CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
651651
DefineConstants(isolate, exports);
652652
} else if (!strcmp(*module_v, "natives")) {
653-
exports = builtins::BuiltinLoader::GetSourceObject(context);
653+
exports = realm->env()->builtin_loader()->GetSourceObject(context);
654654
// Legacy feature: process.binding('natives').config contains stringified
655655
// config.gypi
656656
CHECK(exports
657657
->Set(context,
658658
realm->isolate_data()->config_string(),
659-
builtins::BuiltinLoader::GetConfigString(isolate))
659+
realm->env()->builtin_loader()->GetConfigString(isolate))
660660
.FromJust());
661661
} else {
662662
return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL