From 150b8f7fda526b52f07070b25f404f0b417d6d27 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 21 Sep 2017 01:50:07 +0200 Subject: src: minor c++ refactors to module_wrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move `module_map` to `Environment` instead of having it be global state - `std::map` → `std::unordered_map` - Remove one level of indirection for the map values - Clean up empty vectors in `module_map` - Call `Reset()` on all persistent handles in `resolve_cache_` - Add a missing `HandleScope` to `ModuleWrap::~ModuleWrap()` PR-URL: https://github.com/nodejs/node/pull/15515 Reviewed-By: James M Snell Reviewed-By: Bradley Farias Reviewed-By: Timothy Gu --- src/env.h | 6 ++++++ src/module_wrap.cc | 51 +++++++++++++++++++++++---------------------------- src/module_wrap.h | 6 ++---- 3 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/env.h b/src/env.h index cc2e2d987b..25db2e14e9 100644 --- a/src/env.h +++ b/src/env.h @@ -52,6 +52,10 @@ namespace performance { struct performance_state; } +namespace loader { +class ModuleWrap; +} + // Pick an index that's hopefully out of the way when we're embedded inside // another application. Performance-wise or memory-wise it doesn't matter: // Context::SetAlignedPointerInEmbedderData() is backed by a FixedArray, @@ -585,6 +589,8 @@ class Environment { // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_ids_list(); + std::unordered_multimap module_map; + inline double* heap_statistics_buffer() const; inline void set_heap_statistics_buffer(double* pointer); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 294bf9fa9a..73d248c373 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -18,6 +18,7 @@ using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::HandleScope; using v8::Integer; using v8::IntegrityLevel; using v8::Isolate; @@ -27,32 +28,31 @@ using v8::Maybe; using v8::MaybeLocal; using v8::Module; using v8::Object; -using v8::Persistent; using v8::Promise; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; using v8::Value; -static const char* EXTENSIONS[] = {".mjs", ".js", ".json", ".node"}; -std::map*> ModuleWrap::module_map_; +static const char* const EXTENSIONS[] = {".mjs", ".js", ".json", ".node"}; ModuleWrap::ModuleWrap(Environment* env, Local object, Local module, Local url) : BaseObject(env, object) { - Isolate* iso = Isolate::GetCurrent(); - module_.Reset(iso, module); - url_.Reset(iso, url); + module_.Reset(env->isolate(), module); + url_.Reset(env->isolate(), url); } ModuleWrap::~ModuleWrap() { - Local module = module_.Get(Isolate::GetCurrent()); - std::vector* same_hash = module_map_[module->GetIdentityHash()]; - auto it = std::find(same_hash->begin(), same_hash->end(), this); - - if (it != same_hash->end()) { - same_hash->erase(it); + HandleScope scope(env()->isolate()); + Local module = module_.Get(env()->isolate()); + auto range = env()->module_map.equal_range(module->GetIdentityHash()); + for (auto it = range.first; it != range.second; ++it) { + if (it->second == this) { + env()->module_map.erase(it); + break; + } } module_.Reset(); @@ -120,12 +120,7 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { ModuleWrap* obj = new ModuleWrap(Environment::GetCurrent(ctx), that, mod, url); - if (ModuleWrap::module_map_.count(mod->GetIdentityHash()) == 0) { - ModuleWrap::module_map_[mod->GetIdentityHash()] = - new std::vector(); - } - - ModuleWrap::module_map_[mod->GetIdentityHash()]->push_back(obj); + env->module_map.emplace(mod->GetIdentityHash(), obj); Wrap(that, obj); that->SetIntegrityLevel(ctx, IntegrityLevel::kFrozen); @@ -171,8 +166,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo& args) { env->ThrowError("linking error, expected resolver to return a promise"); } Local resolve_promise = resolve_return_value.As(); - obj->resolve_cache_[specifier_std] = new Persistent(); - obj->resolve_cache_[specifier_std]->Reset(iso, resolve_promise); + obj->resolve_cache_[specifier_std].Reset(env->isolate(), resolve_promise); } args.GetReturnValue().Set(handle_scope.Escape(that)); @@ -188,6 +182,8 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo& args) { Maybe ok = mod->InstantiateModule(ctx, ModuleWrap::ResolveCallback); // clear resolve cache on instantiate + for (auto& entry : obj->resolve_cache_) + entry.second.Reset(); obj->resolve_cache_.clear(); if (!ok.FromMaybe(false)) { @@ -215,18 +211,17 @@ MaybeLocal ModuleWrap::ResolveCallback(Local context, Local referrer) { Environment* env = Environment::GetCurrent(context); Isolate* iso = Isolate::GetCurrent(); - if (ModuleWrap::module_map_.count(referrer->GetIdentityHash()) == 0) { + if (env->module_map.count(referrer->GetIdentityHash()) == 0) { env->ThrowError("linking error, unknown module"); return MaybeLocal(); } - std::vector* possible_deps = - ModuleWrap::module_map_[referrer->GetIdentityHash()]; ModuleWrap* dependent = nullptr; - - for (auto possible_dep : *possible_deps) { - if (possible_dep->module_ == referrer) { - dependent = possible_dep; + auto range = env->module_map.equal_range(referrer->GetIdentityHash()); + for (auto it = range.first; it != range.second; ++it) { + if (it->second->module_ == referrer) { + dependent = it->second; + break; } } @@ -244,7 +239,7 @@ MaybeLocal ModuleWrap::ResolveCallback(Local context, } Local resolve_promise = - dependent->resolve_cache_[specifier_std]->Get(iso); + dependent->resolve_cache_[specifier_std].Get(iso); if (resolve_promise->State() != Promise::kFulfilled) { env->ThrowError("linking error, dependency promises must be resolved on " diff --git a/src/module_wrap.h b/src/module_wrap.h index c669834c6f..0d8b41552b 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -3,7 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include +#include #include #include #include "node_url.h" @@ -45,9 +45,7 @@ class ModuleWrap : public BaseObject { v8::Persistent module_; v8::Persistent url_; bool linked_ = false; - std::map*> resolve_cache_; - - static std::map*> module_map_; + std::unordered_map> resolve_cache_; }; } // namespace loader -- cgit v1.2.1