Skip to content

Commit 91e60b0

Browse files
ofrobotsMylesBorins
authored andcommitted
deps: V8: cherry-pick b49206d from upstream
Original commit message: ThreadDataTable: Change global linked list to per-Isolate hash map. For use cases with a large number of threads or a large number of isolates (or both), ThreadDataTable can be a major performance bottleneck due to O(n) lookup time of the linked list. Switching to a hash map reduces this to O(1). Example 1: Sandstorm.io, a Node.js app that utilizes "fibers", was observed spending the majority of CPU time iterating over the ThreadDataTable. See: https://sandstorm.io/news/2016-09-30-fiber-bomb-debugging-story Example 2: Cloudflare's Workers engine, a high-multi-tenancy web server framework built on V8 (but not Node), creates large numbers of threads and isolates per-process. It saw a 34x improvement in throughput when we applied this patch. Cloudflare has been using a patch in production since the Worker launch which replaces the linked list with a hash map -- but still global. This commit builds on that but goes further and creates a separate hash map and mutex for each isolate, with the table being a member of the Isolate class. This avoids any globals and should reduce lock contention. Bug: v8:5338 Change-Id: If0d11509afb2e043b888c376e36d3463db931b47 Reviewed-on: https://chromium-review.googlesource.com/1014407 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#52753} PR-URL: #20727 Ref: #20083 Refs: #20083 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent e1ff587 commit 91e60b0

File tree

4 files changed

+44
-68
lines changed

4 files changed

+44
-68
lines changed

common.gypi

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727

2828
# Reset this number to 0 on major V8 upgrades.
2929
# Increment by one for each non-official patch applied to deps/v8.
30-
'v8_embedder_string': '-node.7',
30+
'v8_embedder_string': '-node.8',
3131

3232
# Enable disassembler for `--print-code` v8 options
3333
'v8_enable_disassembler': 1,

deps/v8/src/isolate.cc

+26-56
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
#include <fstream> // NOLINT(readability/streams)
1010
#include <sstream>
11+
#include <unordered_map>
1112

1213
#include "src/api.h"
1314
#include "src/assembler-inl.h"
@@ -136,8 +137,6 @@ void ThreadLocalTop::Free() {
136137
base::Thread::LocalStorageKey Isolate::isolate_key_;
137138
base::Thread::LocalStorageKey Isolate::thread_id_key_;
138139
base::Thread::LocalStorageKey Isolate::per_isolate_thread_data_key_;
139-
base::LazyMutex Isolate::thread_data_table_mutex_ = LAZY_MUTEX_INITIALIZER;
140-
Isolate::ThreadDataTable* Isolate::thread_data_table_ = nullptr;
141140
base::Atomic32 Isolate::isolate_counter_ = 0;
142141
#if DEBUG
143142
base::Atomic32 Isolate::isolate_key_created_ = 0;
@@ -148,13 +147,13 @@ Isolate::PerIsolateThreadData*
148147
ThreadId thread_id = ThreadId::Current();
149148
PerIsolateThreadData* per_thread = nullptr;
150149
{
151-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
152-
per_thread = thread_data_table_->Lookup(this, thread_id);
150+
base::LockGuard<base::Mutex> lock_guard(&thread_data_table_mutex_);
151+
per_thread = thread_data_table_.Lookup(thread_id);
153152
if (per_thread == nullptr) {
154153
per_thread = new PerIsolateThreadData(this, thread_id);
155-
thread_data_table_->Insert(per_thread);
154+
thread_data_table_.Insert(per_thread);
156155
}
157-
DCHECK(thread_data_table_->Lookup(this, thread_id) == per_thread);
156+
DCHECK(thread_data_table_.Lookup(thread_id) == per_thread);
158157
}
159158
return per_thread;
160159
}
@@ -165,12 +164,11 @@ void Isolate::DiscardPerThreadDataForThisThread() {
165164
if (thread_id_int) {
166165
ThreadId thread_id = ThreadId(thread_id_int);
167166
DCHECK(!thread_manager_->mutex_owner_.Equals(thread_id));
168-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
169-
PerIsolateThreadData* per_thread =
170-
thread_data_table_->Lookup(this, thread_id);
167+
base::LockGuard<base::Mutex> lock_guard(&thread_data_table_mutex_);
168+
PerIsolateThreadData* per_thread = thread_data_table_.Lookup(thread_id);
171169
if (per_thread) {
172170
DCHECK(!per_thread->thread_state_);
173-
thread_data_table_->Remove(per_thread);
171+
thread_data_table_.Remove(per_thread);
174172
}
175173
}
176174
}
@@ -186,23 +184,20 @@ Isolate::PerIsolateThreadData* Isolate::FindPerThreadDataForThread(
186184
ThreadId thread_id) {
187185
PerIsolateThreadData* per_thread = nullptr;
188186
{
189-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
190-
per_thread = thread_data_table_->Lookup(this, thread_id);
187+
base::LockGuard<base::Mutex> lock_guard(&thread_data_table_mutex_);
188+
per_thread = thread_data_table_.Lookup(thread_id);
191189
}
192190
return per_thread;
193191
}
194192

195193

196194
void Isolate::InitializeOncePerProcess() {
197-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
198-
CHECK_NULL(thread_data_table_);
199195
isolate_key_ = base::Thread::CreateThreadLocalKey();
200196
#if DEBUG
201197
base::Relaxed_Store(&isolate_key_created_, 1);
202198
#endif
203199
thread_id_key_ = base::Thread::CreateThreadLocalKey();
204200
per_isolate_thread_data_key_ = base::Thread::CreateThreadLocalKey();
205-
thread_data_table_ = new Isolate::ThreadDataTable();
206201
}
207202

208203
Address Isolate::get_address_from_id(IsolateAddressId id) {
@@ -2240,14 +2235,9 @@ char* Isolate::RestoreThread(char* from) {
22402235
return from + sizeof(ThreadLocalTop);
22412236
}
22422237

2243-
Isolate::ThreadDataTable::ThreadDataTable() : list_(nullptr) {}
2238+
Isolate::ThreadDataTable::ThreadDataTable() : table_() {}
22442239

2245-
Isolate::ThreadDataTable::~ThreadDataTable() {
2246-
// TODO(svenpanne) The assertion below would fire if an embedder does not
2247-
// cleanly dispose all Isolates before disposing v8, so we are conservative
2248-
// and leave it out for now.
2249-
// DCHECK_NULL(list_);
2250-
}
2240+
Isolate::ThreadDataTable::~ThreadDataTable() {}
22512241

22522242
void Isolate::ReleaseManagedObjects() {
22532243
Isolate::ManagedObjectFinalizer* current =
@@ -2294,40 +2284,30 @@ Isolate::PerIsolateThreadData::~PerIsolateThreadData() {
22942284
#endif
22952285
}
22962286

2297-
2298-
Isolate::PerIsolateThreadData*
2299-
Isolate::ThreadDataTable::Lookup(Isolate* isolate,
2300-
ThreadId thread_id) {
2301-
for (PerIsolateThreadData* data = list_; data != nullptr;
2302-
data = data->next_) {
2303-
if (data->Matches(isolate, thread_id)) return data;
2304-
}
2305-
return nullptr;
2287+
Isolate::PerIsolateThreadData* Isolate::ThreadDataTable::Lookup(
2288+
ThreadId thread_id) {
2289+
auto t = table_.find(thread_id);
2290+
if (t == table_.end()) return nullptr;
2291+
return t->second;
23062292
}
23072293

23082294

23092295
void Isolate::ThreadDataTable::Insert(Isolate::PerIsolateThreadData* data) {
2310-
if (list_ != nullptr) list_->prev_ = data;
2311-
data->next_ = list_;
2312-
list_ = data;
2296+
bool inserted = table_.insert(std::make_pair(data->thread_id_, data)).second;
2297+
CHECK(inserted);
23132298
}
23142299

23152300

23162301
void Isolate::ThreadDataTable::Remove(PerIsolateThreadData* data) {
2317-
if (list_ == data) list_ = data->next_;
2318-
if (data->next_ != nullptr) data->next_->prev_ = data->prev_;
2319-
if (data->prev_ != nullptr) data->prev_->next_ = data->next_;
2302+
table_.erase(data->thread_id_);
23202303
delete data;
23212304
}
23222305

2323-
2324-
void Isolate::ThreadDataTable::RemoveAllThreads(Isolate* isolate) {
2325-
PerIsolateThreadData* data = list_;
2326-
while (data != nullptr) {
2327-
PerIsolateThreadData* next = data->next_;
2328-
if (data->isolate() == isolate) Remove(data);
2329-
data = next;
2306+
void Isolate::ThreadDataTable::RemoveAllThreads() {
2307+
for (auto& x : table_) {
2308+
delete x.second;
23302309
}
2310+
table_.clear();
23312311
}
23322312

23332313

@@ -2502,10 +2482,6 @@ Isolate::Isolate(bool enable_serializer)
25022482
cancelable_task_manager_(new CancelableTaskManager()),
25032483
abort_on_uncaught_exception_callback_(nullptr),
25042484
total_regexp_code_generated_(0) {
2505-
{
2506-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
2507-
CHECK(thread_data_table_);
2508-
}
25092485
id_ = base::Relaxed_AtomicIncrement(&isolate_counter_, 1);
25102486
TRACE_ISOLATE(constructor);
25112487

@@ -2563,8 +2539,8 @@ void Isolate::TearDown() {
25632539
Deinit();
25642540

25652541
{
2566-
base::LockGuard<base::Mutex> lock_guard(thread_data_table_mutex_.Pointer());
2567-
thread_data_table_->RemoveAllThreads(this);
2542+
base::LockGuard<base::Mutex> lock_guard(&thread_data_table_mutex_);
2543+
thread_data_table_.RemoveAllThreads();
25682544
}
25692545

25702546
#ifdef DEBUG
@@ -2578,12 +2554,6 @@ void Isolate::TearDown() {
25782554
}
25792555

25802556

2581-
void Isolate::GlobalTearDown() {
2582-
delete thread_data_table_;
2583-
thread_data_table_ = nullptr;
2584-
}
2585-
2586-
25872557
void Isolate::ClearSerializerData() {
25882558
delete external_reference_table_;
25892559
external_reference_table_ = nullptr;

deps/v8/src/isolate.h

+17-10
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <cstddef>
99
#include <memory>
1010
#include <queue>
11+
#include <unordered_map>
1112
#include <vector>
1213

1314
#include "include/v8.h"
@@ -247,6 +248,8 @@ class ThreadId {
247248
return *this;
248249
}
249250

251+
bool operator==(const ThreadId& other) const { return Equals(other); }
252+
250253
// Returns ThreadId for current thread.
251254
static ThreadId Current() { return ThreadId(GetCurrentThreadId()); }
252255

@@ -287,7 +290,6 @@ class ThreadId {
287290
friend class Isolate;
288291
};
289292

290-
291293
#define FIELD_ACCESSOR(type, name) \
292294
inline void set_##name(type v) { name##_ = v; } \
293295
inline type name() const { return name##_; }
@@ -550,8 +552,6 @@ class Isolate {
550552

551553
void ReleaseManagedObjects();
552554

553-
static void GlobalTearDown();
554-
555555
void ClearSerializerData();
556556

557557
// Find the PerThread for this particular (isolate, thread) combination
@@ -1371,20 +1371,24 @@ class Isolate {
13711371
void* embedder_data_[Internals::kNumIsolateDataSlots];
13721372
Heap heap_;
13731373

1374-
// The per-process lock should be acquired before the ThreadDataTable is
1375-
// modified.
13761374
class ThreadDataTable {
13771375
public:
13781376
ThreadDataTable();
13791377
~ThreadDataTable();
13801378

1381-
PerIsolateThreadData* Lookup(Isolate* isolate, ThreadId thread_id);
1379+
PerIsolateThreadData* Lookup(ThreadId thread_id);
13821380
void Insert(PerIsolateThreadData* data);
13831381
void Remove(PerIsolateThreadData* data);
1384-
void RemoveAllThreads(Isolate* isolate);
1382+
void RemoveAllThreads();
13851383

13861384
private:
1387-
PerIsolateThreadData* list_;
1385+
struct Hasher {
1386+
std::size_t operator()(const ThreadId& t) const {
1387+
return std::hash<int>()(t.ToInteger());
1388+
}
1389+
};
1390+
1391+
std::unordered_map<ThreadId, PerIsolateThreadData*, Hasher> table_;
13881392
};
13891393

13901394
// These items form a stack synchronously with threads Enter'ing and Exit'ing
@@ -1412,12 +1416,15 @@ class Isolate {
14121416
DISALLOW_COPY_AND_ASSIGN(EntryStackItem);
14131417
};
14141418

1415-
static base::LazyMutex thread_data_table_mutex_;
1419+
// TODO([email protected]): This mutex can be removed if
1420+
// thread_data_table_ is always accessed under the isolate lock. I do not
1421+
// know if this is the case, so I'm preserving it for now.
1422+
base::Mutex thread_data_table_mutex_;
14161423

14171424
static base::Thread::LocalStorageKey per_isolate_thread_data_key_;
14181425
static base::Thread::LocalStorageKey isolate_key_;
14191426
static base::Thread::LocalStorageKey thread_id_key_;
1420-
static ThreadDataTable* thread_data_table_;
1427+
ThreadDataTable thread_data_table_;
14211428

14221429
// A global counter for all generated Isolates, might overflow.
14231430
static base::Atomic32 isolate_counter_;

deps/v8/src/v8.cc

-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ void V8::TearDown() {
4949
Bootstrapper::TearDownExtensions();
5050
ElementsAccessor::TearDown();
5151
RegisteredExtension::UnregisterAll();
52-
Isolate::GlobalTearDown();
5352
sampler::Sampler::TearDown();
5453
FlagList::ResetAllFlags(); // Frees memory held by string arguments.
5554
}

0 commit comments

Comments
 (0)