Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e2c47cd

Browse filesBrowse files
addaleaxRafaelGSS
addaleax
authored and
RafaelGSS
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 bbf9da8 commit e2c47cd
Copy full SHA for e2c47cd
Expand file treeCollapse file tree

15 files changed

+347
-173
lines changed
Open diff view settings
Collapse file

‎src/api/environment.cc‎

Copy file name to clipboardExpand all lines: src/api/environment.cc
+10-3Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -473,7 +473,7 @@ MaybeLocal<Value> LoadEnvironment(
473473
return LoadEnvironment(
474474
env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
475475
std::string name = "embedder_main_" + std::to_string(env->thread_id());
476-
builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
476+
env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
477477
Realm* realm = env->principal_realm();
478478

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

717+
// We do not have access to a per-Environment BuiltinLoader instance
718+
// at this point, because this code runs before an Environment exists
719+
// in the first place. However, creating BuiltinLoader instances is
720+
// relatively cheap and all the scripts that we may want to run at
721+
// startup are always present in it.
722+
thread_local builtins::BuiltinLoader builtin_loader;
717723
for (const char** module = context_files; *module != nullptr; module++) {
718724
Local<Value> arguments[] = {exports, primordials};
719-
if (builtins::BuiltinLoader::CompileAndCall(
720-
context, *module, arraysize(arguments), arguments, nullptr)
725+
if (builtin_loader
726+
.CompileAndCall(
727+
context, *module, arraysize(arguments), arguments, nullptr)
721728
.IsEmpty()) {
722729
// Execution failed during context creation.
723730
return Nothing<bool>();
Collapse file

‎src/env-inl.h‎

Copy file name to clipboardExpand all lines: src/env-inl.h
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,10 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
430430
return &destroy_async_id_list_;
431431
}
432432

433+
inline builtins::BuiltinLoader* Environment::builtin_loader() {
434+
return &builtin_loader_;
435+
}
436+
433437
inline double Environment::new_async_id() {
434438
async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
435439
return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];
Collapse file

‎src/env.cc‎

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

Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -719,6 +719,8 @@ class Environment : public MemoryRetainer {
719719
// List of id's that have been destroyed and need the destroy() cb called.
720720
inline std::vector<double>* destroy_async_id_list();
721721

722+
builtins::BuiltinLoader* builtin_loader();
723+
722724
std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
723725
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
724726
std::unordered_map<uint32_t, contextify::ContextifyScript*>
@@ -1134,6 +1136,8 @@ class Environment : public MemoryRetainer {
11341136

11351137
std::unique_ptr<Realm> principal_realm_ = nullptr;
11361138

1139+
builtins::BuiltinLoader builtin_loader_;
1140+
11371141
// Used by allocate_managed_buffer() and release_managed_buffer() to keep
11381142
// track of the BackingStore for a given pointer.
11391143
std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
-5Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,6 @@
126126

127127
namespace node {
128128

129-
using builtins::BuiltinLoader;
130-
131129
using v8::EscapableHandleScope;
132130
using v8::Isolate;
133131
using v8::Local;
@@ -1183,9 +1181,6 @@ ExitCode LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
11831181
}
11841182
}
11851183

1186-
if ((*snapshot_data_ptr) != nullptr) {
1187-
BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
1188-
}
11891184
NodeMainInstance main_instance(*snapshot_data_ptr,
11901185
uv_default_loop(),
11911186
per_process::v8_platform.Platform(),
Collapse file

‎src/node_binding.cc‎

Copy file name to clipboardExpand all lines: src/node_binding.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,13 +631,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
631631
CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
632632
DefineConstants(isolate, exports);
633633
} else if (!strcmp(*module_v, "natives")) {
634-
exports = builtins::BuiltinLoader::GetSourceObject(context);
634+
exports = realm->env()->builtin_loader()->GetSourceObject(context);
635635
// Legacy feature: process.binding('natives').config contains stringified
636636
// config.gypi
637637
CHECK(exports
638638
->Set(context,
639639
realm->isolate_data()->config_string(),
640-
builtins::BuiltinLoader::GetConfigString(isolate))
640+
realm->env()->builtin_loader()->GetConfigString(isolate))
641641
.FromJust());
642642
} else {
643643
return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.