Skip to content

Commit

Permalink
src: refactor bookkeeping of bootstrap status
Browse files Browse the repository at this point in the history 
This patch

1. Refactors the bootstrap routine of the main instance so that
  when --no-node-snapshot is used,
  Environment::InitializeMainContext() will only be called once
  (previously it would be called twice, which was harmless for now
  but not ideal).
2. Mark the number of BaseObjects in RunBootstrapping() when creating
  the Environment from scratch and in InitializeMainContext() when
  the Environment is deserialized. Previously the marking was done in
  the Environment constructor and InitializeMainContext() respectively
  for the cctest which was incorrect because the cctest never uses
  an Environment that's not bootstrapped. Also renames the mark
  to base_object_created_after_bootstrap to reflect what it's
  intended for.

PR-URL: #37113
Refs: #36943
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@joyeecheung
joyeecheung committed Feb 5, 2021
1 parent 664cce9 commit 9aeb836
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 49 deletions.
14 changes: 11 additions & 3 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -780,8 +780,12 @@ inline bool Environment::has_run_bootstrapping_code() const {
return has_run_bootstrapping_code_;
}

inline void Environment::set_has_run_bootstrapping_code(bool value) {
has_run_bootstrapping_code_ = value;
inline void Environment::DoneBootstrapping() {
has_run_bootstrapping_code_ = true;
// This adjusts the return value of base_object_created_after_bootstrap() so
// that tests that check the count do not have to account for internally
// created BaseObjects.
base_object_created_by_bootstrap_ = base_object_count_;
}

inline bool Environment::has_serialized_options() const {
Expand Down Expand Up @@ -1085,8 +1089,12 @@ void Environment::modify_base_object_count(int64_t delta) {
base_object_count_ += delta;
}

int64_t Environment::base_object_created_after_bootstrap() const {
return base_object_count_ - base_object_created_by_bootstrap_;
}

int64_t Environment::base_object_count() const {
return base_object_count_ - initial_base_object_count_;
return base_object_count_;
}

void Environment::set_main_utf16(std::unique_ptr<v8::String::Value> str) {
Expand Down
9 changes: 0 additions & 9 deletions src/env.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,10 +414,6 @@ Environment::Environment(IsolateData* isolate_data,
"args",
std::move(traced_value));
}

// This adjusts the return value of base_object_count() so that tests that
// check the count do not have to account for internally created BaseObjects.
initial_base_object_count_ = base_object_count();
}

Environment::Environment(IsolateData* isolate_data,
Expand Down Expand Up @@ -460,10 +456,6 @@ void Environment::InitializeMainContext(Local<Context> context,
per_process::node_start_time);
performance_state_->Mark(performance::NODE_PERFORMANCE_MILESTONE_V8_START,
performance::performance_v8_start);

// This adjusts the return value of base_object_count() so that tests that
// check the count do not have to account for internally created BaseObjects.
initial_base_object_count_ = base_object_count();
}

Environment::~Environment() {
Expand Down Expand Up @@ -654,7 +646,6 @@ void Environment::RunCleanup() {
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
"RunCleanup", this);
bindings_.clear();
initial_base_object_count_ = 0;
CleanupHandles();

while (!cleanup_hooks_.empty() ||
Expand Down
5 changes: 3 additions & 2 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,7 @@ class Environment : public MemoryRetainer {
inline void add_refs(int64_t diff);

inline bool has_run_bootstrapping_code() const;
inline void set_has_run_bootstrapping_code(bool has_run_bootstrapping_code);
inline void DoneBootstrapping();

inline bool has_serialized_options() const;
inline void set_has_serialized_options(bool has_serialized_options);
Expand Down Expand Up @@ -1315,6 +1315,7 @@ class Environment : public MemoryRetainer {
// no memory leaks caused by BaseObjects staying alive longer than expected
// (in particular, no circular BaseObjectPtr references).
inline void modify_base_object_count(int64_t delta);
inline int64_t base_object_created_after_bootstrap() const;
inline int64_t base_object_count() const;

inline int32_t stack_trace_limit() const { return 10; }
Expand Down Expand Up @@ -1508,7 +1509,7 @@ class Environment : public MemoryRetainer {
bool started_cleanup_ = false;

int64_t base_object_count_ = 0;
int64_t initial_base_object_count_ = 0;
int64_t base_object_created_by_bootstrap_ = 0;
std::atomic_bool is_stopping_ { false };

std::unordered_set<int> unmanaged_fds_;
Expand Down
2 changes: 1 addition & 1 deletion src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ MaybeLocal<Value> Environment::RunBootstrapping() {
CHECK(req_wrap_queue()->IsEmpty());
CHECK(handle_wrap_queue()->IsEmpty());

set_has_run_bootstrapping_code(true);
DoneBootstrapping();

return scope.Escape(result);
}
Expand Down
24 changes: 12 additions & 12 deletions src/node_main_instance.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,10 +209,18 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code,
{DeserializeNodeInternalFields, env.get()})
.ToLocalChecked();

CHECK(!context.IsEmpty());
Context::Scope context_scope(context);
InitializeContextRuntime(context);
SetIsolateErrorHandlers(isolate_, {});
env->InitializeMainContext(context, env_info);
#if HAVE_INSPECTOR
env->InitializeInspector({});
#endif
env->DoneBootstrapping();
} else {
context = NewContext(isolate_);
CHECK(!context.IsEmpty());
Context::Scope context_scope(context);
env.reset(new Environment(isolate_data_.get(),
context,
Expand All @@ -221,24 +229,16 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code,
nullptr,
EnvironmentFlags::kDefaultFlags,
{}));
}

CHECK(!context.IsEmpty());
Context::Scope context_scope(context);

env->InitializeMainContext(context, env_info);

#if HAVE_INSPECTOR
env->InitializeInspector({});
env->InitializeInspector({});
#endif

if (!deserialize_mode_ && env->RunBootstrapping().IsEmpty()) {
return nullptr;
if (env->RunBootstrapping().IsEmpty()) {
return nullptr;
}
}

CHECK(env->req_wrap_queue()->IsEmpty());
CHECK(env->handle_wrap_queue()->IsEmpty());
env->set_has_run_bootstrapping_code(true);
return env;
}

Expand Down
38 changes: 16 additions & 22 deletions test/cctest/test_base_object_ptr.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ using v8::Isolate;
using v8::Local;
using v8::Object;

// Environments may come with existing BaseObject instances.
// This variable offsets the expected BaseObject counts.
static const int BASE_OBJECT_COUNT = 0;

class BaseObjectPtrTest : public EnvironmentTestFixture {};

class DummyBaseObject : public BaseObject {
Expand Down Expand Up @@ -51,12 +47,12 @@ TEST_F(BaseObjectPtrTest, ScopedDetached) {
Env env_{handle_scope, argv};
Environment* env = *env_;

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
{
BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::NewDetached(env);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
}
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
}

TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) {
Expand All @@ -67,14 +63,14 @@ TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) {

BaseObjectWeakPtr<DummyBaseObject> weak_ptr;

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
{
BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::NewDetached(env);
weak_ptr = ptr;
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
}
EXPECT_EQ(weak_ptr.get(), nullptr);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
}

TEST_F(BaseObjectPtrTest, Undetached) {
Expand All @@ -86,13 +82,12 @@ TEST_F(BaseObjectPtrTest, Undetached) {
node::AddEnvironmentCleanupHook(
isolate_,
[](void* arg) {
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(),
BASE_OBJECT_COUNT);
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(), 0);
},
env);

BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::New(env);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
}

TEST_F(BaseObjectPtrTest, GCWeak) {
Expand All @@ -109,21 +104,21 @@ TEST_F(BaseObjectPtrTest, GCWeak) {
weak_ptr = ptr;
ptr->MakeWeak();

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
EXPECT_EQ(weak_ptr.get(), ptr.get());
EXPECT_EQ(weak_ptr->persistent().IsWeak(), false);

ptr.reset();
}

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
EXPECT_NE(weak_ptr.get(), nullptr);
EXPECT_EQ(weak_ptr->persistent().IsWeak(), true);

v8::V8::SetFlagsFromString("--expose-gc");
isolate_->RequestGarbageCollectionForTesting(Isolate::kFullGarbageCollection);

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
EXPECT_EQ(weak_ptr.get(), nullptr);
}

Expand All @@ -134,7 +129,7 @@ TEST_F(BaseObjectPtrTest, Moveable) {
Environment* env = *env_;

BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::NewDetached(env);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
BaseObjectWeakPtr<DummyBaseObject> weak_ptr { ptr };
EXPECT_EQ(weak_ptr.get(), ptr.get());

Expand All @@ -145,12 +140,12 @@ TEST_F(BaseObjectPtrTest, Moveable) {
BaseObjectWeakPtr<DummyBaseObject> weak_ptr2 = std::move(weak_ptr);
EXPECT_EQ(weak_ptr2.get(), ptr2.get());
EXPECT_EQ(weak_ptr.get(), nullptr);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);

ptr2.reset();

EXPECT_EQ(weak_ptr2.get(), nullptr);
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
}

TEST_F(BaseObjectPtrTest, NestedClasses) {
Expand All @@ -174,8 +169,7 @@ TEST_F(BaseObjectPtrTest, NestedClasses) {
node::AddEnvironmentCleanupHook(
isolate_,
[](void* arg) {
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(),
BASE_OBJECT_COUNT);
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(), 0);
},
env);

Expand All @@ -184,5 +178,5 @@ TEST_F(BaseObjectPtrTest, NestedClasses) {
obj->ptr1 = DummyBaseObject::NewDetached(env);
obj->ptr2 = DummyBaseObject::New(env);

EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 3);
EXPECT_EQ(env->base_object_created_after_bootstrap(), 3);
}

0 comments on commit 9aeb836

Please sign in to comment.