Skip to content

Commit ad5d8e3

Browse files
addaleaxtargos
authored andcommitted
src: refactor thread stopping mechanism
- Follow style guide for naming, e.g. use lower_snake_case for simple setters/getters. - For performance, use atomics instead of a mutex, and inline the corresponding getter/setter pair. PR-URL: #26757 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1 parent db7c4ac commit ad5d8e3

File tree

5 files changed

+29
-33
lines changed

5 files changed

+29
-33
lines changed

src/env-inl.h

+9-1
Original file line numberDiff line numberDiff line change
@@ -709,7 +709,7 @@ inline void Environment::remove_sub_worker_context(worker::Worker* context) {
709709
}
710710

711711
inline bool Environment::is_stopping() const {
712-
return thread_stopper_.IsStopped();
712+
return thread_stopper_.is_stopped();
713713
}
714714

715715
inline performance::performance_state* Environment::performance_state() {
@@ -979,6 +979,14 @@ void Environment::ForEachBaseObject(T&& iterator) {
979979
}
980980
}
981981

982+
bool AsyncRequest::is_stopped() const {
983+
return stopped_.load();
984+
}
985+
986+
void AsyncRequest::set_stopped(bool flag) {
987+
stopped_.store(flag);
988+
}
989+
982990
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
983991
#define VY(PropertyName, StringValue) V(v8::Symbol, PropertyName)
984992
#define VS(PropertyName, StringValue) V(v8::String, PropertyName)

src/env.cc

+7-22
Original file line numberDiff line numberDiff line change
@@ -350,13 +350,13 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
350350
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
351351
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
352352

353-
GetAsyncRequest()->Install(
353+
thread_stopper()->Install(
354354
this, static_cast<void*>(this), [](uv_async_t* handle) {
355355
Environment* env = static_cast<Environment*>(handle->data);
356356
uv_stop(env->event_loop());
357357
});
358-
GetAsyncRequest()->SetStopped(false);
359-
uv_unref(reinterpret_cast<uv_handle_t*>(GetAsyncRequest()->GetHandle()));
358+
thread_stopper()->set_stopped(false);
359+
uv_unref(reinterpret_cast<uv_handle_t*>(thread_stopper()->GetHandle()));
360360

361361
// Register clean-up cb to be called to clean up the handles
362362
// when the environment is freed, note that they are not cleaned in
@@ -375,7 +375,7 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
375375

376376
void Environment::ExitEnv() {
377377
set_can_call_into_js(false);
378-
GetAsyncRequest()->Stop();
378+
thread_stopper()->Stop();
379379
isolate_->TerminateExecution();
380380
}
381381

@@ -543,7 +543,7 @@ void Environment::RunCleanup() {
543543
started_cleanup_ = true;
544544
TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
545545
"RunCleanup", this);
546-
GetAsyncRequest()->Uninstall();
546+
thread_stopper()->Uninstall();
547547
CleanupHandles();
548548

549549
while (!cleanup_hooks_.empty()) {
@@ -977,49 +977,34 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
977977
}
978978

979979
void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
980-
Mutex::ScopedLock lock(mutex_);
980+
CHECK_NULL(async_);
981981
env_ = env;
982982
async_ = new uv_async_t;
983983
async_->data = data;
984984
CHECK_EQ(uv_async_init(env_->event_loop(), async_, target), 0);
985985
}
986986

987987
void AsyncRequest::Uninstall() {
988-
Mutex::ScopedLock lock(mutex_);
989988
if (async_ != nullptr) {
990989
env_->CloseHandle(async_, [](uv_async_t* async) { delete async; });
991990
async_ = nullptr;
992991
}
993992
}
994993

995994
void AsyncRequest::Stop() {
996-
Mutex::ScopedLock lock(mutex_);
997-
stop_ = true;
995+
set_stopped(true);
998996
if (async_ != nullptr) uv_async_send(async_);
999997
}
1000998

1001-
void AsyncRequest::SetStopped(bool flag) {
1002-
Mutex::ScopedLock lock(mutex_);
1003-
stop_ = flag;
1004-
}
1005-
1006-
bool AsyncRequest::IsStopped() const {
1007-
Mutex::ScopedLock lock(mutex_);
1008-
return stop_;
1009-
}
1010-
1011999
uv_async_t* AsyncRequest::GetHandle() {
1012-
Mutex::ScopedLock lock(mutex_);
10131000
return async_;
10141001
}
10151002

10161003
void AsyncRequest::MemoryInfo(MemoryTracker* tracker) const {
1017-
Mutex::ScopedLock lock(mutex_);
10181004
if (async_ != nullptr) tracker->TrackField("async_request", *async_);
10191005
}
10201006

10211007
AsyncRequest::~AsyncRequest() {
1022-
Mutex::ScopedLock lock(mutex_);
10231008
CHECK_NULL(async_);
10241009
}
10251010

src/env.h

+7-5
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "uv.h"
3939
#include "v8.h"
4040

41+
#include <atomic>
4142
#include <cstdint>
4243
#include <functional>
4344
#include <list>
@@ -515,18 +516,19 @@ class AsyncRequest : public MemoryRetainer {
515516
void Install(Environment* env, void* data, uv_async_cb target);
516517
void Uninstall();
517518
void Stop();
518-
void SetStopped(bool flag);
519-
bool IsStopped() const;
519+
inline void set_stopped(bool flag);
520+
inline bool is_stopped() const;
520521
uv_async_t* GetHandle();
521522
void MemoryInfo(MemoryTracker* tracker) const override;
523+
524+
522525
SET_MEMORY_INFO_NAME(AsyncRequest)
523526
SET_SELF_SIZE(AsyncRequest)
524527

525528
private:
526529
Environment* env_;
527530
uv_async_t* async_ = nullptr;
528-
mutable Mutex mutex_;
529-
bool stop_ = true;
531+
std::atomic_bool stopped_ {true};
530532
};
531533

532534
class Environment {
@@ -1051,7 +1053,7 @@ class Environment {
10511053
inline ExecutionMode execution_mode() { return execution_mode_; }
10521054

10531055
inline void set_execution_mode(ExecutionMode mode) { execution_mode_ = mode; }
1054-
inline AsyncRequest* GetAsyncRequest() { return &thread_stopper_; }
1056+
inline AsyncRequest* thread_stopper() { return &thread_stopper_; }
10551057

10561058
private:
10571059
inline void CreateImmediate(native_immediate_callback cb,

src/node.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -824,14 +824,14 @@ inline int StartNodeWithIsolate(Isolate* isolate,
824824
per_process::v8_platform.DrainVMTasks(isolate);
825825

826826
more = uv_loop_alive(env.event_loop());
827-
if (more && !env.GetAsyncRequest()->IsStopped()) continue;
827+
if (more && !env.is_stopping()) continue;
828828

829829
RunBeforeExit(&env);
830830

831831
// Emit `beforeExit` if the loop became alive either after emitting
832832
// event, or after running some callbacks.
833833
more = uv_loop_alive(env.event_loop());
834-
} while (more == true && !env.GetAsyncRequest()->IsStopped());
834+
} while (more == true && !env.is_stopping());
835835
env.performance_state()->Mark(
836836
node::performance::NODE_PERFORMANCE_MILESTONE_LOOP_EXIT);
837837
}

src/node_worker.cc

+4-3
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ Worker::Worker(Environment* env,
103103
bool Worker::is_stopped() const {
104104
Mutex::ScopedLock lock(mutex_);
105105
if (env_ != nullptr)
106-
return env_->GetAsyncRequest()->IsStopped();
106+
return env_->is_stopping();
107107
return stopped_;
108108
}
109109

@@ -217,7 +217,7 @@ void Worker::Run() {
217217
stopped_ = true;
218218
this->env_ = nullptr;
219219
}
220-
env_->GetAsyncRequest()->SetStopped(true);
220+
env_->thread_stopper()->set_stopped(true);
221221
env_->stop_sub_worker_contexts();
222222
env_->RunCleanup();
223223
RunAtExit(env_.get());
@@ -376,7 +376,8 @@ void Worker::OnThreadStopped() {
376376
Worker::~Worker() {
377377
Mutex::ScopedLock lock(mutex_);
378378

379-
CHECK(stopped_ || env_ == nullptr || env_->GetAsyncRequest()->IsStopped());
379+
CHECK(stopped_);
380+
CHECK_NULL(env_);
380381
CHECK(thread_joined_);
381382

382383
Debug(this, "Worker %llu destroyed", thread_id_);

0 commit comments

Comments
 (0)