Skip to content

Commit

Permalink
src: minor c++ refactors to module_wrap
Browse files Browse the repository at this point in the history
- 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: #15515
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
  • Loading branch information
addaleax authored and jasnell committed Sep 25, 2017
1 parent 17d8dfe commit 90d14df
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 32 deletions.
6 changes: 6 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -586,6 +590,8 @@ class Environment {
// List of id's that have been destroyed and need the destroy() cb called.
inline std::vector<double>* destroy_ids_list();

std::unordered_multimap<int, loader::ModuleWrap*> module_map;

inline double* heap_statistics_buffer() const;
inline void set_heap_statistics_buffer(double* pointer);

Expand Down
51 changes: 23 additions & 28 deletions src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,32 +27,31 @@ using v8::Local;
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<int, std::vector<ModuleWrap*>*> ModuleWrap::module_map_;
static const char* const EXTENSIONS[] = {".mjs", ".js", ".json", ".node"};

ModuleWrap::ModuleWrap(Environment* env,
Local<Object> object,
Local<Module> module,
Local<String> 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 = module_.Get(Isolate::GetCurrent());
std::vector<ModuleWrap*>* 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 = 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();
Expand Down Expand Up @@ -119,12 +119,7 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& 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*>();
}

ModuleWrap::module_map_[mod->GetIdentityHash()]->push_back(obj);
env->module_map.emplace(mod->GetIdentityHash(), obj);
Wrap(that, obj);

that->SetIntegrityLevel(ctx, IntegrityLevel::kFrozen);
Expand Down Expand Up @@ -170,8 +165,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
env->ThrowError("linking error, expected resolver to return a promise");
}
Local<Promise> resolve_promise = resolve_return_value.As<Promise>();
obj->resolve_cache_[specifier_std] = new Persistent<Promise>();
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));
Expand All @@ -187,6 +181,8 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
bool ok = mod->Instantiate(ctx, ModuleWrap::ResolveCallback);

// clear resolve cache on instantiate
for (auto& entry : obj->resolve_cache_)
entry.second.Reset();
obj->resolve_cache_.clear();

if (!ok) {
Expand Down Expand Up @@ -214,18 +210,17 @@ MaybeLocal<Module> ModuleWrap::ResolveCallback(Local<Context> context,
Local<Module> 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<Module>();
}

std::vector<ModuleWrap*>* 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;
}
}

Expand All @@ -243,7 +238,7 @@ MaybeLocal<Module> ModuleWrap::ResolveCallback(Local<Context> context,
}

Local<Promise> 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 "
Expand Down
6 changes: 2 additions & 4 deletions src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <map>
#include <unordered_map>
#include <string>
#include <vector>
#include "node_url.h"
Expand Down Expand Up @@ -45,9 +45,7 @@ class ModuleWrap : public BaseObject {
v8::Persistent<v8::Module> module_;
v8::Persistent<v8::String> url_;
bool linked_ = false;
std::map<std::string, v8::Persistent<v8::Promise>*> resolve_cache_;

static std::map<int, std::vector<ModuleWrap*>*> module_map_;
std::unordered_map<std::string, v8::Persistent<v8::Promise>> resolve_cache_;
};

} // namespace loader
Expand Down

0 comments on commit 90d14df

Please sign in to comment.