Skip to content

Commit f301a23

Browse files
committed
deps: V8: backport 75289aadf9c8
Original commit message: add Isolate::Deinitialize() and Isolate::Free() This allows embedders to mirror the isolate disposal routine with an initialization routine that uses Isolate::Allocate(). ``` v8::Isolate* isolate = v8::Isolate::Allocate(); // Use the isolate address as a key. v8::Isolate::Initialize(isolate, params); isolate->Deinitialize(); // Remove the entry keyed by isolate address. v8::Isolate::Free(isolate); ``` Previously, the only way to dispose the isolate bundles the de-initialization and the freeing of the address together in v8::Isolate::Dispose(). This is inadequate for embedders like Node.js that uses the isolate address as a key to manage the task runner associated with it, if another thread gets an isolate allocated at the aligned address before the other thread finishes cleanup for the isolate previously allocated at the same address, and locking on the entire disposal can be too risky since it may post GC tasks that in turn requires using the isolate address to locate the task runner. It's a lot simpler to handle the issue if the disposal process of the isolate can mirror the initialization of it and split into two routines. Refs: nodejs#57753 (comment) Refs: nodejs#30850 Bug: 412943769 Change-Id: I3865c27395aded3a6f32de74d96d0698b2d891b9 Refs: v8/v8@75289aa
1 parent e185daa commit f301a23

File tree

6 files changed

+74
-8
lines changed

6 files changed

+74
-8
lines changed

common.gypi

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939
# Reset this number to 0 on major V8 upgrades.
4040
# Increment by one for each non-official patch applied to deps/v8.
41-
'v8_embedder_string': '-node.10',
41+
'v8_embedder_string': '-node.11',
4242

4343
##### V8 defaults for Node.js #####
4444

deps/v8/include/v8-isolate.h

+16-1
Original file line numberDiff line numberDiff line change
@@ -872,11 +872,26 @@ class V8_EXPORT Isolate {
872872
void Exit();
873873

874874
/**
875-
* Disposes the isolate. The isolate must not be entered by any
875+
* Deinitializes and frees the isolate. The isolate must not be entered by any
876876
* thread to be disposable.
877877
*/
878878
void Dispose();
879879

880+
/**
881+
* Deinitializes the isolate, but does not free the address. The isolate must
882+
* not be entered by any thread to be deinitializable. Embedders must call
883+
* Isolate::Free() to free the isolate afterwards.
884+
*/
885+
void Deinitialize();
886+
887+
/**
888+
* Frees the memory allocated for the isolate. Can only be called after the
889+
* Isolate has already been deinitialized with Isolate::Deinitialize(). After
890+
* the isolate is freed, the next call to Isolate::New() or
891+
* Isolate::Allocate() might return the same address that just get freed.
892+
*/
893+
static void Free(Isolate* isolate);
894+
880895
/**
881896
* Dumps activated low-level V8 internal stats. This can be used instead
882897
* of performing a full isolate disposal.

deps/v8/src/api/api.cc

+15
Original file line numberDiff line numberDiff line change
@@ -10132,6 +10132,21 @@ void Isolate::Dispose() {
1013210132
i::Isolate::Delete(i_isolate);
1013310133
}
1013410134

10135+
void Isolate::Deinitialize() {
10136+
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(this);
10137+
if (!Utils::ApiCheck(
10138+
!i_isolate->IsInUse(), "v8::Isolate::Deinitialize()",
10139+
"Deinitializing the isolate that is entered by a thread")) {
10140+
return;
10141+
}
10142+
i::Isolate::Deinitialize(i_isolate);
10143+
}
10144+
10145+
void Isolate::Free(Isolate* isolate) {
10146+
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
10147+
i::Isolate::Free(i_isolate);
10148+
}
10149+
1013510150
void Isolate::DumpAndResetStats() {
1013610151
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(this);
1013710152
i_isolate->DumpAndResetStats();

deps/v8/src/execution/isolate.cc

+13-2
Original file line numberDiff line numberDiff line change
@@ -4133,6 +4133,12 @@ Isolate* Isolate::Allocate(IsolateGroup* group) {
41334133

41344134
// static
41354135
void Isolate::Delete(Isolate* isolate) {
4136+
Deinitialize(isolate);
4137+
Free(isolate);
4138+
}
4139+
4140+
// static
4141+
void Isolate::Deinitialize(Isolate* isolate) {
41364142
DCHECK_NOT_NULL(isolate);
41374143
// v8::V8::Dispose() must only be called after deleting all isolates.
41384144
DCHECK_NOT_NULL(V8::GetCurrentPlatform());
@@ -4156,13 +4162,18 @@ void Isolate::Delete(Isolate* isolate) {
41564162
isolate->~Isolate();
41574163
// Only release the group once all other Isolate members have been destroyed.
41584164
group->Release();
4159-
// Free the isolate itself.
4160-
base::AlignedFree(isolate);
41614165

41624166
// Restore the previous current isolate.
41634167
SetIsolateThreadLocals(saved_isolate, saved_data);
41644168
}
41654169

4170+
// static
4171+
void Isolate::Free(Isolate* isolate) {
4172+
DCHECK_NOT_NULL(isolate);
4173+
// Free the memory allocated for the Isolate.
4174+
base::AlignedFree(isolate);
4175+
}
4176+
41664177
void Isolate::SetUpFromReadOnlyArtifacts(ReadOnlyArtifacts* artifacts) {
41674178
DCHECK_NOT_NULL(artifacts);
41684179
InitializeNextUniqueSfiId(artifacts->initial_next_unique_sfi_id());

deps/v8/src/execution/isolate.h

+10-4
Original file line numberDiff line numberDiff line change
@@ -661,10 +661,16 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
661661
static Isolate* New();
662662
static Isolate* New(IsolateGroup* isolate_group);
663663

664-
// Deletes Isolate object. Must be used instead of delete operator.
665-
// Destroys the non-default isolates.
666-
// Sets default isolate into "has_been_disposed" state rather then destroying,
667-
// for legacy API reasons.
664+
// Deinitialize Isolate object. Must be used instead of delete operator.
665+
// Destroys the non-default isolates. Sets default isolate into
666+
// "has_been_disposed" state rather then destroying, for legacy API reasons.
667+
// Another Free() call must be done after the isolate is deinitialized.
668+
// This gives the embedder a chance to clean up before the address is released
669+
// (which maybe reused in the next allocation).
670+
static void Deinitialize(Isolate* isolate);
671+
// Frees the address of the isolate. Must be called after Deinitialize().
672+
static void Free(Isolate* isolate);
673+
// A convenience helper that deinitializes and frees the isolate.
668674
static void Delete(Isolate* isolate);
669675

670676
void SetUpFromReadOnlyArtifacts(ReadOnlyArtifacts* artifacts);

deps/v8/test/cctest/test-api.cc

+19
Original file line numberDiff line numberDiff line change
@@ -31344,3 +31344,22 @@ TEST(LocalCasts) {
3134431344
v8::MaybeLocal<v8::String>::Cast(no_data);
3134531345
}
3134631346
}
31347+
31348+
TEST(IsolateFree) {
31349+
v8::Isolate* isolate1 = v8::Isolate::Allocate();
31350+
v8::Isolate::Initialize(isolate1, CreateTestParams());
31351+
isolate1->Deinitialize();
31352+
31353+
// When a new isolate is allocated immediately after one is freed, there is
31354+
// a chance that the new isolate will be allocated at the same address as the
31355+
// old one. This tests that when Deinitialize() is called, allocating a new
31356+
// isolate does not reuse the old address.
31357+
v8::Isolate* isolate2 = v8::Isolate::Allocate();
31358+
CHECK_NE(isolate1, isolate2);
31359+
31360+
v8::Isolate::Initialize(isolate2, CreateTestParams());
31361+
isolate2->Deinitialize();
31362+
31363+
v8::Isolate::Free(isolate1);
31364+
v8::Isolate::Free(isolate2);
31365+
}

0 commit comments

Comments
 (0)