Skip to content

Commit 1e4b71d

Browse files
nickieV8 LUCI CQ
nickie
authored and
V8 LUCI CQ
committed
[heap] Move the Stack object from ThreadLocalTop to Isolate
Stack information is thread-specific and, until now, it was stored in a field in ThreadLocalTop. This CL moves stack information to the isolate and makes sure to update the stack start whenever a main thread enters the isolate. At the same time, the Stack object is refactored and simplified. As a side effect, after removing the Stack object, ThreadLocalTop satisfies the std::standard_layout trait; this fixes some issues observed with different C++ compilers. Bug: v8:13630 Bug: v8:13257 Change-Id: I026a35af3bc6999a09b21f277756d4454c086343 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4152476 Reviewed-by: Michael Lippautz <[email protected]> Reviewed-by: Omer Katz <[email protected]> Commit-Queue: Nikolaos Papaspyrou <[email protected]> Cr-Commit-Position: refs/heads/main@{#85445}
1 parent 14de33a commit 1e4b71d

9 files changed

Lines changed: 62 additions & 43 deletions

File tree

‎src/execution/isolate.cc‎

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3069,21 +3069,23 @@ void Isolate::AddSharedWasmMemory(Handle<WasmMemoryObject> memory_object) {
30693069
void Isolate::RecordStackSwitchForScanning() {
30703070
Object current = root(RootIndex::kActiveContinuation);
30713071
DCHECK(!current.IsUndefined());
3072-
thread_local_top()->stack_.ClearStackSegments();
3073-
wasm::StackMemory* stack = Managed<wasm::StackMemory>::cast(
3074-
WasmContinuationObject::cast(current).stack())
3075-
.get()
3076-
.get();
3072+
stack().ClearStackSegments();
3073+
wasm::StackMemory* wasm_stack =
3074+
Managed<wasm::StackMemory>::cast(
3075+
WasmContinuationObject::cast(current).stack())
3076+
.get()
3077+
.get();
30773078
current = WasmContinuationObject::cast(current).parent();
3078-
heap()->SetStackStart(reinterpret_cast<void*>(stack->base()));
3079+
heap()->SetStackStart(reinterpret_cast<void*>(wasm_stack->base()));
30793080
// We don't need to add all inactive stacks. Only the ones in the active chain
30803081
// may contain cpp heap pointers.
30813082
while (!current.IsUndefined()) {
30823083
auto cont = WasmContinuationObject::cast(current);
3083-
auto* stack = Managed<wasm::StackMemory>::cast(cont.stack()).get().get();
3084-
thread_local_top()->stack_.AddStackSegment(
3085-
reinterpret_cast<const void*>(stack->base()),
3086-
reinterpret_cast<const void*>(stack->jmpbuf()->sp));
3084+
auto* wasm_stack =
3085+
Managed<wasm::StackMemory>::cast(cont.stack()).get().get();
3086+
stack().AddStackSegment(
3087+
reinterpret_cast<const void*>(wasm_stack->base()),
3088+
reinterpret_cast<const void*>(wasm_stack->jmpbuf()->sp));
30873089
current = cont.parent();
30883090
}
30893091
}
@@ -3371,23 +3373,13 @@ void Isolate::Delete(Isolate* isolate) {
33713373
Isolate* saved_isolate = isolate->TryGetCurrent();
33723374
SetIsolateThreadLocals(isolate, nullptr);
33733375
isolate->set_thread_id(ThreadId::Current());
3374-
if (saved_isolate) {
3375-
isolate->thread_local_top()->stack_ =
3376-
std::move(saved_isolate->thread_local_top()->stack_);
3377-
} else {
3378-
isolate->heap()->SetStackStart(base::Stack::GetStackStart());
3379-
}
3376+
isolate->heap()->SetStackStart(base::Stack::GetStackStart());
33803377

33813378
bool owns_shared_isolate = isolate->owns_shared_isolate_;
33823379
Isolate* maybe_shared_isolate = isolate->shared_isolate_;
33833380

33843381
isolate->Deinit();
33853382

3386-
// Restore the saved isolate's stack.
3387-
if (saved_isolate)
3388-
saved_isolate->thread_local_top()->stack_ =
3389-
std::move(isolate->thread_local_top()->stack_);
3390-
33913383
#ifdef DEBUG
33923384
non_disposed_isolates_--;
33933385
#endif // DEBUG
@@ -4618,6 +4610,10 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
46184610
void Isolate::Enter() {
46194611
Isolate* current_isolate = nullptr;
46204612
PerIsolateThreadData* current_data = CurrentPerIsolateThreadData();
4613+
4614+
// Set the stack start for the main thread that enters the isolate.
4615+
heap()->SetStackStart(base::Stack::GetStackStart());
4616+
46214617
if (current_data != nullptr) {
46224618
current_isolate = current_data->isolate_;
46234619
DCHECK_NOT_NULL(current_isolate);

‎src/execution/isolate.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "src/execution/stack-guard.h"
3333
#include "src/handles/handles.h"
3434
#include "src/handles/traced-handles.h"
35+
#include "src/heap/base/stack.h"
3536
#include "src/heap/factory.h"
3637
#include "src/heap/heap.h"
3738
#include "src/heap/read-only-heap.h"
@@ -2029,6 +2030,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
20292030
SimulatorData* simulator_data() { return simulator_data_; }
20302031
#endif
20312032

2033+
::heap::base::Stack& stack() { return stack_; }
2034+
20322035
#ifdef V8_ENABLE_WEBASSEMBLY
20332036
wasm::StackMemory*& wasm_stacks() { return wasm_stacks_; }
20342037
// Update the thread local's Stack object so that it is aware of the new stack
@@ -2527,6 +2530,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
25272530
// The mutex only guards adding pages, the retrieval is signal safe.
25282531
base::Mutex code_pages_mutex_;
25292532

2533+
// Stack information for the main thread.
2534+
::heap::base::Stack stack_;
2535+
25302536
#ifdef V8_ENABLE_WEBASSEMBLY
25312537
wasm::StackMemory* wasm_stacks_;
25322538
#endif

‎src/execution/thread-local-top.cc‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,20 +37,15 @@ void ThreadLocalTop::Clear() {
3737
current_embedder_state_ = nullptr;
3838
failed_access_check_callback_ = nullptr;
3939
thread_in_wasm_flag_address_ = kNullAddress;
40-
stack_ = ::heap::base::Stack();
4140
}
4241

4342
void ThreadLocalTop::Initialize(Isolate* isolate) {
4443
Clear();
4544
isolate_ = isolate;
4645
thread_id_ = ThreadId::Current();
4746
#if V8_ENABLE_WEBASSEMBLY
48-
stack_.SetStackStart(base::Stack::GetStackStart(),
49-
v8_flags.experimental_wasm_stack_switching);
5047
thread_in_wasm_flag_address_ = reinterpret_cast<Address>(
5148
trap_handler::GetThreadInWasmThreadLocalAddress());
52-
#else
53-
stack_.SetStackStart(base::Stack::GetStackStart(), false);
5449
#endif // V8_ENABLE_WEBASSEMBLY
5550
#ifdef USE_SIMULATOR
5651
simulator_ = Simulator::current(isolate);

‎src/execution/thread-local-top.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "include/v8-unwinder.h"
1111
#include "src/common/globals.h"
1212
#include "src/execution/thread-id.h"
13-
#include "src/heap/base/stack.h"
1413
#include "src/objects/contexts.h"
1514
#include "src/utils/utils.h"
1615

@@ -30,7 +29,7 @@ class ThreadLocalTop {
3029
// TODO(all): This is not particularly beautiful. We should probably
3130
// refactor this to really consist of just Addresses and 32-bit
3231
// integer fields.
33-
static constexpr uint32_t kSizeInBytes = 30 * kSystemPointerSize;
32+
static constexpr uint32_t kSizeInBytes = 25 * kSystemPointerSize;
3433

3534
// Does early low-level initialization that does not depend on the
3635
// isolate being present.
@@ -147,9 +146,6 @@ class ThreadLocalTop {
147146

148147
// Address of the thread-local "thread in wasm" flag.
149148
Address thread_in_wasm_flag_address_;
150-
151-
// Stack information.
152-
::heap::base::Stack stack_;
153149
};
154150

155151
} // namespace internal

‎src/heap/heap.cc‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5813,9 +5813,7 @@ void Heap::SetStackStart(void* stack_start) {
58135813
#endif // V8_ENABLE_WEBASSEMBLY
58145814
}
58155815

5816-
::heap::base::Stack& Heap::stack() {
5817-
return isolate_->thread_local_top()->stack_;
5818-
}
5816+
::heap::base::Stack& Heap::stack() { return isolate_->stack(); }
58195817

58205818
void Heap::StartTearDown() {
58215819
// Finish any ongoing sweeping to avoid stray background tasks still accessing

‎test/cctest/test-api.cc‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13043,12 +13043,13 @@ bool ApiTestFuzzer::NextThread() {
1304313043

1304413044

1304513045
void ApiTestFuzzer::Run() {
13046-
// When it is our turn...
13046+
// Wait until it is our turn.
1304713047
gate_.Wait();
1304813048
{
13049-
// ... get the V8 lock
13049+
// Get the V8 lock.
1305013050
v8::Locker locker(CcTest::isolate());
13051-
// ... and start running the test.
13051+
// Start running the test, which will enter the isolate and exit it when it
13052+
// finishes.
1305213053
CallTest();
1305313054
}
1305413055
// This test finished.
@@ -13089,11 +13090,18 @@ static void CallTestNumber(int test_number) {
1308913090

1309013091

1309113092
void ApiTestFuzzer::RunAllTests() {
13093+
// This method is called when running each THREADING_TEST, which is an
13094+
// initialized test and has entered the isolate at this point. We need to exit
13095+
// the isolate, so that the fuzzer threads can enter it in turn, while running
13096+
// their tests.
13097+
CcTest::isolate()->Exit();
1309213098
// Set off the first test.
1309313099
current_ = -1;
1309413100
NextThread();
1309513101
// Wait till they are all done.
1309613102
all_tests_done_.Wait();
13103+
// We enter the isolate again, to prepare for teardown.
13104+
CcTest::isolate()->Enter();
1309713105
}
1309813106

1309913107

@@ -13111,10 +13119,16 @@ int ApiTestFuzzer::GetNextTestNumber() {
1311113119
void ApiTestFuzzer::ContextSwitch() {
1311213120
// If the new thread is the same as the current thread there is nothing to do.
1311313121
if (NextThread()) {
13114-
// Now it can start.
13115-
v8::Unlocker unlocker(CcTest::isolate());
13116-
// Wait till someone starts us again.
13117-
gate_.Wait();
13122+
// Exit the isolate from this thread.
13123+
CcTest::i_isolate()->Exit();
13124+
{
13125+
// Now the new thread can start.
13126+
v8::Unlocker unlocker(CcTest::isolate());
13127+
// Wait till someone starts us again.
13128+
gate_.Wait();
13129+
}
13130+
// Enter the isolate from this thread again.
13131+
CcTest::i_isolate()->Enter();
1311813132
// And we're off.
1311913133
}
1312013134
}

‎test/cctest/test-cpu-profiler.cc‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3457,8 +3457,12 @@ TEST(MultipleThreadsSingleIsolate) {
34573457
env, "YieldIsolate", [](const v8::FunctionCallbackInfo<v8::Value>& info) {
34583458
v8::Isolate* isolate = info.GetIsolate();
34593459
if (!info[0]->IsTrue()) return;
3460-
v8::Unlocker unlocker(isolate);
3461-
v8::base::OS::Sleep(v8::base::TimeDelta::FromMilliseconds(1));
3460+
isolate->Exit();
3461+
{
3462+
v8::Unlocker unlocker(isolate);
3463+
v8::base::OS::Sleep(v8::base::TimeDelta::FromMilliseconds(1));
3464+
}
3465+
isolate->Enter();
34623466
});
34633467

34643468
CompileRun(varying_frame_size_script);
@@ -3470,11 +3474,13 @@ TEST(MultipleThreadsSingleIsolate) {
34703474

34713475
// For good measure, profile on our own thread
34723476
UnlockingThread::Profile(env, 0);
3477+
isolate->Exit();
34733478
{
34743479
v8::Unlocker unlocker(isolate);
34753480
thread1.Join();
34763481
thread2.Join();
34773482
}
3483+
isolate->Enter();
34783484
}
34793485

34803486
// Tests that StopProfiling doesn't wait for the next sample tick in order to

‎test/cctest/test-debug.cc‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4211,6 +4211,12 @@ class ArchiveRestoreThread : public v8::base::Thread,
42114211
// child.GetBreakCount() will return 1 if the debugger fails to stop
42124212
// on the `next()` line after the grandchild thread returns.
42134213
CHECK_EQ(child.GetBreakCount(), 5);
4214+
4215+
// This test on purpose unlocks the isolate without exiting and
4216+
// re-entering. It must however update the stack start, which would have
4217+
// been done automatically if the isolate was properly re-entered.
4218+
reinterpret_cast<i::Isolate*>(isolate_)->heap()->SetStackStart(
4219+
v8::base::Stack::GetStackStart());
42144220
}
42154221
}
42164222

‎test/cctest/test-lockers.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -871,7 +871,9 @@ TEST(LockUnlockLockDefaultIsolateMultithreaded) {
871871
threads.push_back(new LockUnlockLockDefaultIsolateThread(context));
872872
}
873873
}
874+
CcTest::isolate()->Exit();
874875
StartJoinAndDeleteThreads(threads);
876+
CcTest::isolate()->Enter();
875877
}
876878

877879

0 commit comments

Comments
 (0)