Skip to content

Commit

Permalink
src: track ShadowRealm native objects correctly in the heap snapshot
Browse files Browse the repository at this point in the history
Previously the native ShadowRealm objects were never actually tracked
in the heap snapshot - the tracking starts with the Environment,
we don't call the tracking methods unless it's reachable natively from
the Environment. This patch adds a set in the Environment for tracking
shadow realms in the heap snapshot.

Drive-by: remove redundant MemoryInfo() overrides from the derived
classes of Realm and remove the tracking of `env` from
`Realm::MemoryInfo()` because the realms don't hold the Environment
alive (even for the principal realm, it's the other way around).

PR-URL: #47389
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
joyeecheung authored and RafaelGSS committed Apr 13, 2023
1 parent 9a4d21d commit 5250947
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 9 deletions.
14 changes: 14 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "node_internals.h"
#include "node_options-inl.h"
#include "node_process-inl.h"
#include "node_shadow_realm.h"
#include "node_v8_platform-inl.h"
#include "node_worker.h"
#include "req_wrap-inl.h"
Expand Down Expand Up @@ -226,6 +227,14 @@ void Environment::UntrackContext(Local<Context> context) {
}
}

void Environment::TrackShadowRealm(shadow_realm::ShadowRealm* realm) {
shadow_realms_.insert(realm);
}

void Environment::UntrackShadowRealm(shadow_realm::ShadowRealm* realm) {
shadow_realms_.erase(realm);
}

AsyncHooks::DefaultTriggerAsyncIdScope::DefaultTriggerAsyncIdScope(
Environment* env, double default_trigger_async_id)
: async_hooks_(env->async_hooks()) {
Expand Down Expand Up @@ -901,6 +910,10 @@ Environment::~Environment() {
addon.Close();
}
}

for (auto realm : shadow_realms_) {
realm->OnEnvironmentDestruct();
}
}

void Environment::InitializeLibuv() {
Expand Down Expand Up @@ -1896,6 +1909,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const {
tracker->TrackField("timeout_info", timeout_info_);
tracker->TrackField("tick_info", tick_info_);
tracker->TrackField("principal_realm", principal_realm_);
tracker->TrackField("shadow_realms", shadow_realms_);

// FIXME(joyeecheung): track other fields in Environment.
// Currently MemoryTracker is unable to track these
Expand Down
6 changes: 6 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@

namespace node {

namespace shadow_realm {
class ShadowRealm;
}
namespace contextify {
class ContextifyScript;
class CompiledFnEntry;
Expand Down Expand Up @@ -643,6 +646,8 @@ class Environment : public MemoryRetainer {
const ContextInfo& info);
void TrackContext(v8::Local<v8::Context> context);
void UntrackContext(v8::Local<v8::Context> context);
void TrackShadowRealm(shadow_realm::ShadowRealm* realm);
void UntrackShadowRealm(shadow_realm::ShadowRealm* realm);

void StartProfilerIdleNotifier();

Expand Down Expand Up @@ -1020,6 +1025,7 @@ class Environment : public MemoryRetainer {

size_t async_callback_scope_depth_ = 0;
std::vector<double> destroy_async_id_list_;
std::unordered_set<shadow_realm::ShadowRealm*> shadow_realms_;

#if HAVE_INSPECTOR
std::unique_ptr<profiler::V8CoverageConnection> coverage_connection_;
Expand Down
5 changes: 0 additions & 5 deletions src/node_realm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ void Realm::MemoryInfo(MemoryTracker* tracker) const {
PER_REALM_STRONG_PERSISTENT_VALUES(V)
#undef V

tracker->TrackField("env", env_);
tracker->TrackField("cleanup_queue", cleanup_queue_);
tracker->TrackField("builtins_with_cache", builtins_with_cache);
tracker->TrackField("builtins_without_cache", builtins_without_cache);
Expand Down Expand Up @@ -301,10 +300,6 @@ PrincipalRealm::PrincipalRealm(Environment* env,
}
}

void PrincipalRealm::MemoryInfo(MemoryTracker* tracker) const {
Realm::MemoryInfo(tracker);
}

MaybeLocal<Value> PrincipalRealm::BootstrapRealm() {
HandleScope scope(isolate_);

Expand Down
1 change: 0 additions & 1 deletion src/node_realm.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ class PrincipalRealm : public Realm {

SET_MEMORY_INFO_NAME(PrincipalRealm)
SET_SELF_SIZE(PrincipalRealm)
void MemoryInfo(MemoryTracker* tracker) const override;

#define V(PropertyName, TypeName) \
v8::Local<TypeName> PropertyName() const override; \
Expand Down
10 changes: 8 additions & 2 deletions src/node_shadow_realm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& data) {

ShadowRealm::ShadowRealm(Environment* env)
: Realm(env, NewContext(env->isolate()), kShadowRealm) {
env->TrackShadowRealm(this);
context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
CreateProperties();
}
Expand All @@ -54,10 +55,15 @@ ShadowRealm::~ShadowRealm() {
while (HasCleanupHooks()) {
RunCleanup();
}
if (env_ != nullptr) {
env_->UntrackShadowRealm(this);
}
}

void ShadowRealm::MemoryInfo(MemoryTracker* tracker) const {
Realm::MemoryInfo(tracker);
void ShadowRealm::OnEnvironmentDestruct() {
CHECK_NOT_NULL(env_);
env_ = nullptr; // This means that the shadow realm has out-lived the
// environment.
}

v8::Local<v8::Context> ShadowRealm::context() const {
Expand Down
3 changes: 2 additions & 1 deletion src/node_shadow_realm.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class ShadowRealm : public Realm {

SET_MEMORY_INFO_NAME(ShadowRealm)
SET_SELF_SIZE(ShadowRealm)
void MemoryInfo(MemoryTracker* tracker) const override;

v8::Local<v8::Context> context() const override;

Expand All @@ -25,6 +24,8 @@ class ShadowRealm : public Realm {
PER_REALM_STRONG_PERSISTENT_VALUES(V)
#undef V

void OnEnvironmentDestruct();

protected:
v8::MaybeLocal<v8::Value> BootstrapRealm() override;

Expand Down

0 comments on commit 5250947

Please sign in to comment.