From 2a5f67b3812a531f2bfed3330d827da165283163 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Jan 2021 22:00:03 +0800 Subject: [PATCH] src: refactor bookkeeping of bootstrap status 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: https://github.com/nodejs/node/pull/37113 Refs: https://github.com/nodejs/node/pull/36943 Reviewed-By: James M Snell --- src/env-inl.h | 14 ++++++++--- src/env.cc | 9 ------- src/env.h | 5 ++-- src/node.cc | 2 +- src/node_main_instance.cc | 24 +++++++++--------- test/cctest/test_base_object_ptr.cc | 38 ++++++++++++----------------- 6 files changed, 43 insertions(+), 49 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 98badcd0362627..95b818373b9a0a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -784,8 +784,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 { @@ -1089,8 +1093,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 str) { diff --git a/src/env.cc b/src/env.cc index 69d5a33a945d9f..16c6fcc3db4749 100644 --- a/src/env.cc +++ b/src/env.cc @@ -422,10 +422,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, @@ -468,10 +464,6 @@ void Environment::InitializeMainContext(Local 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() { @@ -662,7 +654,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() || diff --git a/src/env.h b/src/env.h index 38c087b650e3ac..f2fc681f56ed69 100644 --- a/src/env.h +++ b/src/env.h @@ -1132,7 +1132,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); @@ -1318,6 +1318,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; } @@ -1511,7 +1512,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 unmanaged_fds_; diff --git a/src/node.cc b/src/node.cc index efb4876002f774..decaa755e39f12 100644 --- a/src/node.cc +++ b/src/node.cc @@ -415,7 +415,7 @@ MaybeLocal Environment::RunBootstrapping() { CHECK(req_wrap_queue()->IsEmpty()); CHECK(handle_wrap_queue()->IsEmpty()); - set_has_run_bootstrapping_code(true); + DoneBootstrapping(); return scope.Escape(result); } diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 84d39831dcd1f7..37d9dbb9b75525 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -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, @@ -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; } diff --git a/test/cctest/test_base_object_ptr.cc b/test/cctest/test_base_object_ptr.cc index a3a8df98a9efc7..52a1e2aff79dde 100644 --- a/test/cctest/test_base_object_ptr.cc +++ b/test/cctest/test_base_object_ptr.cc @@ -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 { @@ -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 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) { @@ -67,14 +63,14 @@ TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) { BaseObjectWeakPtr weak_ptr; - EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT); + EXPECT_EQ(env->base_object_created_after_bootstrap(), 0); { BaseObjectPtr 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) { @@ -86,13 +82,12 @@ TEST_F(BaseObjectPtrTest, Undetached) { node::AddEnvironmentCleanupHook( isolate_, [](void* arg) { - EXPECT_EQ(static_cast(arg)->base_object_count(), - BASE_OBJECT_COUNT); + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); }, env); BaseObjectPtr 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) { @@ -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); } @@ -134,7 +129,7 @@ TEST_F(BaseObjectPtrTest, Moveable) { Environment* env = *env_; BaseObjectPtr ptr = DummyBaseObject::NewDetached(env); - EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1); + EXPECT_EQ(env->base_object_created_after_bootstrap(), 1); BaseObjectWeakPtr weak_ptr { ptr }; EXPECT_EQ(weak_ptr.get(), ptr.get()); @@ -145,12 +140,12 @@ TEST_F(BaseObjectPtrTest, Moveable) { BaseObjectWeakPtr 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) { @@ -174,8 +169,7 @@ TEST_F(BaseObjectPtrTest, NestedClasses) { node::AddEnvironmentCleanupHook( isolate_, [](void* arg) { - EXPECT_EQ(static_cast(arg)->base_object_count(), - BASE_OBJECT_COUNT); + EXPECT_EQ(static_cast(arg)->base_object_count(), 0); }, env); @@ -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); }