Skip to content

Commit 2a5f67b

Browse files
joyeecheungdanielleadams
joyeecheung
authored and
danielleadams
committed
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: #37113 Refs: #36943 Reviewed-By: James M Snell <[email protected]>
1 parent acd087d commit 2a5f67b

File tree

6 files changed

+43
-49
lines changed

6 files changed

+43
-49
lines changed

Diff for: ‎src/env-inl.h

+11-3
Original file line numberDiff line numberDiff line change
@@ -784,8 +784,12 @@ inline bool Environment::has_run_bootstrapping_code() const {
784784
return has_run_bootstrapping_code_;
785785
}
786786

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

791795
inline bool Environment::has_serialized_options() const {
@@ -1089,8 +1093,12 @@ void Environment::modify_base_object_count(int64_t delta) {
10891093
base_object_count_ += delta;
10901094
}
10911095

1096+
int64_t Environment::base_object_created_after_bootstrap() const {
1097+
return base_object_count_ - base_object_created_by_bootstrap_;
1098+
}
1099+
10921100
int64_t Environment::base_object_count() const {
1093-
return base_object_count_ - initial_base_object_count_;
1101+
return base_object_count_;
10941102
}
10951103

10961104
void Environment::set_main_utf16(std::unique_ptr<v8::String::Value> str) {

Diff for: ‎src/env.cc

-9
Original file line numberDiff line numberDiff line change
@@ -422,10 +422,6 @@ Environment::Environment(IsolateData* isolate_data,
422422
"args",
423423
std::move(traced_value));
424424
}
425-
426-
// This adjusts the return value of base_object_count() so that tests that
427-
// check the count do not have to account for internally created BaseObjects.
428-
initial_base_object_count_ = base_object_count();
429425
}
430426

431427
Environment::Environment(IsolateData* isolate_data,
@@ -468,10 +464,6 @@ void Environment::InitializeMainContext(Local<Context> context,
468464
per_process::node_start_time);
469465
performance_state_->Mark(performance::NODE_PERFORMANCE_MILESTONE_V8_START,
470466
performance::performance_v8_start);
471-
472-
// This adjusts the return value of base_object_count() so that tests that
473-
// check the count do not have to account for internally created BaseObjects.
474-
initial_base_object_count_ = base_object_count();
475467
}
476468

477469
Environment::~Environment() {
@@ -662,7 +654,6 @@ void Environment::RunCleanup() {
662654
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
663655
"RunCleanup", this);
664656
bindings_.clear();
665-
initial_base_object_count_ = 0;
666657
CleanupHandles();
667658

668659
while (!cleanup_hooks_.empty() ||

Diff for: ‎src/env.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -1132,7 +1132,7 @@ class Environment : public MemoryRetainer {
11321132
inline void add_refs(int64_t diff);
11331133

11341134
inline bool has_run_bootstrapping_code() const;
1135-
inline void set_has_run_bootstrapping_code(bool has_run_bootstrapping_code);
1135+
inline void DoneBootstrapping();
11361136

11371137
inline bool has_serialized_options() const;
11381138
inline void set_has_serialized_options(bool has_serialized_options);
@@ -1318,6 +1318,7 @@ class Environment : public MemoryRetainer {
13181318
// no memory leaks caused by BaseObjects staying alive longer than expected
13191319
// (in particular, no circular BaseObjectPtr references).
13201320
inline void modify_base_object_count(int64_t delta);
1321+
inline int64_t base_object_created_after_bootstrap() const;
13211322
inline int64_t base_object_count() const;
13221323

13231324
inline int32_t stack_trace_limit() const { return 10; }
@@ -1511,7 +1512,7 @@ class Environment : public MemoryRetainer {
15111512
bool started_cleanup_ = false;
15121513

15131514
int64_t base_object_count_ = 0;
1514-
int64_t initial_base_object_count_ = 0;
1515+
int64_t base_object_created_by_bootstrap_ = 0;
15151516
std::atomic_bool is_stopping_ { false };
15161517

15171518
std::unordered_set<int> unmanaged_fds_;

Diff for: ‎src/node.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,7 @@ MaybeLocal<Value> Environment::RunBootstrapping() {
415415
CHECK(req_wrap_queue()->IsEmpty());
416416
CHECK(handle_wrap_queue()->IsEmpty());
417417

418-
set_has_run_bootstrapping_code(true);
418+
DoneBootstrapping();
419419

420420
return scope.Escape(result);
421421
}

Diff for: ‎src/node_main_instance.cc

+12-12
Original file line numberDiff line numberDiff line change
@@ -209,10 +209,18 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code,
209209
{DeserializeNodeInternalFields, env.get()})
210210
.ToLocalChecked();
211211

212+
CHECK(!context.IsEmpty());
213+
Context::Scope context_scope(context);
212214
InitializeContextRuntime(context);
213215
SetIsolateErrorHandlers(isolate_, {});
216+
env->InitializeMainContext(context, env_info);
217+
#if HAVE_INSPECTOR
218+
env->InitializeInspector({});
219+
#endif
220+
env->DoneBootstrapping();
214221
} else {
215222
context = NewContext(isolate_);
223+
CHECK(!context.IsEmpty());
216224
Context::Scope context_scope(context);
217225
env.reset(new Environment(isolate_data_.get(),
218226
context,
@@ -221,24 +229,16 @@ NodeMainInstance::CreateMainEnvironment(int* exit_code,
221229
nullptr,
222230
EnvironmentFlags::kDefaultFlags,
223231
{}));
224-
}
225-
226-
CHECK(!context.IsEmpty());
227-
Context::Scope context_scope(context);
228-
229-
env->InitializeMainContext(context, env_info);
230-
231232
#if HAVE_INSPECTOR
232-
env->InitializeInspector({});
233+
env->InitializeInspector({});
233234
#endif
234-
235-
if (!deserialize_mode_ && env->RunBootstrapping().IsEmpty()) {
236-
return nullptr;
235+
if (env->RunBootstrapping().IsEmpty()) {
236+
return nullptr;
237+
}
237238
}
238239

239240
CHECK(env->req_wrap_queue()->IsEmpty());
240241
CHECK(env->handle_wrap_queue()->IsEmpty());
241-
env->set_has_run_bootstrapping_code(true);
242242
return env;
243243
}
244244

Diff for: ‎test/cctest/test_base_object_ptr.cc

+16-22
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,6 @@ using v8::Isolate;
1414
using v8::Local;
1515
using v8::Object;
1616

17-
// Environments may come with existing BaseObject instances.
18-
// This variable offsets the expected BaseObject counts.
19-
static const int BASE_OBJECT_COUNT = 0;
20-
2117
class BaseObjectPtrTest : public EnvironmentTestFixture {};
2218

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

54-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
50+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
5551
{
5652
BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::NewDetached(env);
57-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
53+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
5854
}
59-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
55+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
6056
}
6157

6258
TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) {
@@ -67,14 +63,14 @@ TEST_F(BaseObjectPtrTest, ScopedDetachedWithWeak) {
6763

6864
BaseObjectWeakPtr<DummyBaseObject> weak_ptr;
6965

70-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
66+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
7167
{
7268
BaseObjectPtr<DummyBaseObject> ptr = DummyBaseObject::NewDetached(env);
7369
weak_ptr = ptr;
74-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT + 1);
70+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 1);
7571
}
7672
EXPECT_EQ(weak_ptr.get(), nullptr);
77-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
73+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
7874
}
7975

8076
TEST_F(BaseObjectPtrTest, Undetached) {
@@ -86,13 +82,12 @@ TEST_F(BaseObjectPtrTest, Undetached) {
8682
node::AddEnvironmentCleanupHook(
8783
isolate_,
8884
[](void* arg) {
89-
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(),
90-
BASE_OBJECT_COUNT);
85+
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(), 0);
9186
},
9287
env);
9388

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

9893
TEST_F(BaseObjectPtrTest, GCWeak) {
@@ -109,21 +104,21 @@ TEST_F(BaseObjectPtrTest, GCWeak) {
109104
weak_ptr = ptr;
110105
ptr->MakeWeak();
111106

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

116111
ptr.reset();
117112
}
118113

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

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

126-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
121+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
127122
EXPECT_EQ(weak_ptr.get(), nullptr);
128123
}
129124

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

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

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

150145
ptr2.reset();
151146

152147
EXPECT_EQ(weak_ptr2.get(), nullptr);
153-
EXPECT_EQ(env->base_object_count(), BASE_OBJECT_COUNT);
148+
EXPECT_EQ(env->base_object_created_after_bootstrap(), 0);
154149
}
155150

156151
TEST_F(BaseObjectPtrTest, NestedClasses) {
@@ -174,8 +169,7 @@ TEST_F(BaseObjectPtrTest, NestedClasses) {
174169
node::AddEnvironmentCleanupHook(
175170
isolate_,
176171
[](void* arg) {
177-
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(),
178-
BASE_OBJECT_COUNT);
172+
EXPECT_EQ(static_cast<Environment*>(arg)->base_object_count(), 0);
179173
},
180174
env);
181175

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

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

0 commit comments

Comments
 (0)