Commit b648738e authored by Lee Howes's avatar Lee Howes Committed by Facebook GitHub Bot

Add executor pointer to guard and log that plus name

Summary: Add a separate tag to the blocking guards, derived by default from the names available in the executors. This allows more fine-grained information when reporting blocking operations on the executors.

Reviewed By: yfeldblum

Differential Revision: D28265296

fbshipit-source-id: 98d877ab13dc73c0b0b06edbdf15ec0e06f4d74a
parent b9f3d9be
...@@ -46,7 +46,7 @@ void Executor::keepAliveRelease() noexcept { ...@@ -46,7 +46,7 @@ void Executor::keepAliveRelease() noexcept {
} }
// Base case of permitting with no termination to avoid nullptr tests // Base case of permitting with no termination to avoid nullptr tests
static ExecutorBlockingList emptyList{nullptr, {false, false, {}}}; static ExecutorBlockingList emptyList{nullptr, {false, false, nullptr, {}}};
thread_local ExecutorBlockingList* executor_blocking_list = &emptyList; thread_local ExecutorBlockingList* executor_blocking_list = &emptyList;
...@@ -61,30 +61,40 @@ ExecutorBlockingGuard::ExecutorBlockingGuard(PermitTag) noexcept { ...@@ -61,30 +61,40 @@ ExecutorBlockingGuard::ExecutorBlockingGuard(PermitTag) noexcept {
list_ = *executor_blocking_list; list_ = *executor_blocking_list;
list_.prev = executor_blocking_list; list_.prev = executor_blocking_list;
list_.curr.forbid = false; list_.curr.forbid = false;
list_.curr.name = {}; // Do not overwrite tag or executor pointer
executor_blocking_list = &list_; executor_blocking_list = &list_;
} }
} }
ExecutorBlockingGuard::ExecutorBlockingGuard( ExecutorBlockingGuard::ExecutorBlockingGuard(
TrackTag, StringPiece name) noexcept { TrackTag, Executor* ex, StringPiece tag) noexcept {
if (!kIsMobile) { if (!kIsMobile) {
list_ = *executor_blocking_list; list_ = *executor_blocking_list;
list_.prev = executor_blocking_list; list_.prev = executor_blocking_list;
list_.curr.forbid = true; list_.curr.forbid = true;
list_.curr.name = name; list_.curr.ex = ex;
// If no string was provided, maintain the parent string to keep some
// information
if (!tag.empty()) {
list_.curr.tag = tag;
}
executor_blocking_list = &list_; executor_blocking_list = &list_;
} }
} }
ExecutorBlockingGuard::ExecutorBlockingGuard( ExecutorBlockingGuard::ExecutorBlockingGuard(
ProhibitTag, StringPiece name) noexcept { ProhibitTag, Executor* ex, StringPiece tag) noexcept {
if (!kIsMobile) { if (!kIsMobile) {
list_ = *executor_blocking_list; list_ = *executor_blocking_list;
list_.prev = executor_blocking_list; list_.prev = executor_blocking_list;
list_.curr.forbid = true; list_.curr.forbid = true;
list_.curr.ex = ex;
list_.curr.allowTerminationOnBlocking = true; list_.curr.allowTerminationOnBlocking = true;
list_.curr.name = name; // If no string was provided, maintain the parent string to keep some
// information
if (!tag.empty()) {
list_.curr.tag = tag;
}
executor_blocking_list = &list_; executor_blocking_list = &list_;
} }
} }
......
...@@ -307,7 +307,8 @@ Executor::KeepAlive<ExecutorT> getKeepAliveToken( ...@@ -307,7 +307,8 @@ Executor::KeepAlive<ExecutorT> getKeepAliveToken(
struct ExecutorBlockingContext { struct ExecutorBlockingContext {
bool forbid; bool forbid;
bool allowTerminationOnBlocking; bool allowTerminationOnBlocking;
StringPiece name; Executor* ex = nullptr;
StringPiece tag;
}; };
static_assert( static_assert(
std::is_standard_layout<ExecutorBlockingContext>::value, std::is_standard_layout<ExecutorBlockingContext>::value,
...@@ -331,8 +332,10 @@ class ExecutorBlockingGuard { ...@@ -331,8 +332,10 @@ class ExecutorBlockingGuard {
ExecutorBlockingGuard() = delete; ExecutorBlockingGuard() = delete;
explicit ExecutorBlockingGuard(PermitTag) noexcept; explicit ExecutorBlockingGuard(PermitTag) noexcept;
explicit ExecutorBlockingGuard(TrackTag, StringPiece name) noexcept; explicit ExecutorBlockingGuard(
explicit ExecutorBlockingGuard(ProhibitTag, StringPiece name) noexcept; TrackTag, Executor* ex, StringPiece tag) noexcept;
explicit ExecutorBlockingGuard(
ProhibitTag, Executor* ex, StringPiece tag) noexcept;
ExecutorBlockingGuard(ExecutorBlockingGuard&&) = delete; ExecutorBlockingGuard(ExecutorBlockingGuard&&) = delete;
ExecutorBlockingGuard(ExecutorBlockingGuard const&) = delete; ExecutorBlockingGuard(ExecutorBlockingGuard const&) = delete;
......
...@@ -41,8 +41,6 @@ namespace { ...@@ -41,8 +41,6 @@ namespace {
using default_queue = UnboundedBlockingQueue<CPUThreadPoolExecutor::CPUTask>; using default_queue = UnboundedBlockingQueue<CPUThreadPoolExecutor::CPUTask>;
using default_queue_alloc = using default_queue_alloc =
AlignedSysAllocator<default_queue, FixedAlign<alignof(default_queue)>>; AlignedSysAllocator<default_queue, FixedAlign<alignof(default_queue)>>;
constexpr folly::StringPiece executorName = "CPUThreadPoolExecutor";
} // namespace } // namespace
const size_t CPUThreadPoolExecutor::kDefaultMaxQueueSize = 1 << 14; const size_t CPUThreadPoolExecutor::kDefaultMaxQueueSize = 1 << 14;
...@@ -268,9 +266,9 @@ void CPUThreadPoolExecutor::threadRun(ThreadPtr thread) { ...@@ -268,9 +266,9 @@ void CPUThreadPoolExecutor::threadRun(ThreadPtr thread) {
this->threadPoolHook_.registerThread(); this->threadPoolHook_.registerThread();
folly::Optional<ExecutorBlockingGuard> guard; // optional until C++17 folly::Optional<ExecutorBlockingGuard> guard; // optional until C++17
if (prohibitBlockingOnThreadPools_ == Options::Blocking::prohibit) { if (prohibitBlockingOnThreadPools_ == Options::Blocking::prohibit) {
guard.emplace(ExecutorBlockingGuard::ProhibitTag{}, executorName); guard.emplace(ExecutorBlockingGuard::ProhibitTag{}, this, namePrefix_);
} else { } else {
guard.emplace(ExecutorBlockingGuard::TrackTag{}, executorName); guard.emplace(ExecutorBlockingGuard::TrackTag{}, this, namePrefix_);
} }
thread->startupBaton.post(); thread->startupBaton.post();
......
...@@ -31,9 +31,6 @@ ...@@ -31,9 +31,6 @@
#include <folly/ScopeGuard.h> #include <folly/ScopeGuard.h>
namespace folly { namespace folly {
namespace {
constexpr folly::StringPiece executorName = "EDFThreadPoolExecutor";
}
class EDFThreadPoolExecutor::Task { class EDFThreadPoolExecutor::Task {
public: public:
...@@ -336,7 +333,8 @@ folly::Executor::KeepAlive<> EDFThreadPoolExecutor::deadlineExecutor( ...@@ -336,7 +333,8 @@ folly::Executor::KeepAlive<> EDFThreadPoolExecutor::deadlineExecutor(
void EDFThreadPoolExecutor::threadRun(ThreadPtr thread) { void EDFThreadPoolExecutor::threadRun(ThreadPtr thread) {
this->threadPoolHook_.registerThread(); this->threadPoolHook_.registerThread();
ExecutorBlockingGuard guard{ExecutorBlockingGuard::TrackTag{}, executorName}; ExecutorBlockingGuard guard{
ExecutorBlockingGuard::TrackTag{}, this, namePrefix_};
thread->startupBaton.post(); thread->startupBaton.post();
for (;;) { for (;;) {
......
...@@ -203,19 +203,23 @@ void IOThreadPoolExecutor::threadRun(ThreadPtr thread) { ...@@ -203,19 +203,23 @@ void IOThreadPoolExecutor::threadRun(ThreadPtr thread) {
ioThread->eventBase->runInEventBaseThread( ioThread->eventBase->runInEventBaseThread(
[thread] { thread->startupBaton.post(); }); [thread] { thread->startupBaton.post(); });
while (ioThread->shouldRun) { {
ioThread->eventBase->loopForever(); ExecutorBlockingGuard guard{
} ExecutorBlockingGuard::TrackTag{}, this, namePrefix_};
if (isJoin_) { while (ioThread->shouldRun) {
while (ioThread->pendingTasks > 0) { ioThread->eventBase->loopForever();
ioThread->eventBase->loopOnce(); }
if (isJoin_) {
while (ioThread->pendingTasks > 0) {
ioThread->eventBase->loopOnce();
}
}
idler.reset();
if (isWaitForAll_) {
// some tasks, like thrift asynchronous calls, create additional
// event base hookups, let's wait till all of them complete.
ioThread->eventBase->loop();
} }
}
idler.reset();
if (isWaitForAll_) {
// some tasks, like thrift asynchronous calls, create additional
// event base hookups, let's wait till all of them complete.
ioThread->eventBase->loop();
} }
std::lock_guard<std::mutex> guard(ioThread->eventBaseShutdownMutex_); std::lock_guard<std::mutex> guard(ioThread->eventBaseShutdownMutex_);
......
...@@ -1033,10 +1033,10 @@ TEST(ThreadPoolExecutorTest, VirtualExecutorTestEDF) { ...@@ -1033,10 +1033,10 @@ TEST(ThreadPoolExecutorTest, VirtualExecutorTestEDF) {
template <class TPE> template <class TPE>
static void currentThreadTest(folly::StringPiece executorName) { static void currentThreadTest(folly::StringPiece executorName) {
folly::Optional<ExecutorBlockingContext> ctx{}; folly::Optional<ExecutorBlockingContext> ctx{};
TPE tpe(1); TPE tpe(1, std::make_shared<NamedThreadFactory>(executorName));
tpe.add([&ctx]() { ctx = getExecutorBlockingContext(); }); tpe.add([&ctx]() { ctx = getExecutorBlockingContext(); });
tpe.join(); tpe.join();
EXPECT_EQ(ctx->name, executorName); EXPECT_EQ(ctx->tag, executorName);
} }
// Test the nesting of the permit guard // Test the nesting of the permit guard
...@@ -1044,7 +1044,7 @@ template <class TPE> ...@@ -1044,7 +1044,7 @@ template <class TPE>
static void currentThreadTestDisabled(folly::StringPiece executorName) { static void currentThreadTestDisabled(folly::StringPiece executorName) {
folly::Optional<ExecutorBlockingContext> ctxPermit{}; folly::Optional<ExecutorBlockingContext> ctxPermit{};
folly::Optional<ExecutorBlockingContext> ctxForbid{}; folly::Optional<ExecutorBlockingContext> ctxForbid{};
TPE tpe(1); TPE tpe(1, std::make_shared<NamedThreadFactory>(executorName));
tpe.add([&]() { tpe.add([&]() {
{ {
// Nest the guard that permits blocking // Nest the guard that permits blocking
...@@ -1055,20 +1055,20 @@ static void currentThreadTestDisabled(folly::StringPiece executorName) { ...@@ -1055,20 +1055,20 @@ static void currentThreadTestDisabled(folly::StringPiece executorName) {
}); });
tpe.join(); tpe.join();
EXPECT_TRUE(!ctxPermit.has_value()); EXPECT_TRUE(!ctxPermit.has_value());
EXPECT_EQ(ctxForbid->name, executorName); EXPECT_EQ(ctxForbid->tag, executorName);
} }
TEST(ThreadPoolExecutorTest, CPUCurrentThreadExecutor) { TEST(ThreadPoolExecutorTest, CPUCurrentThreadExecutor) {
currentThreadTest<CPUThreadPoolExecutor>("CPUThreadPoolExecutor"); currentThreadTest<CPUThreadPoolExecutor>("CPU-ExecutorName");
currentThreadTestDisabled<CPUThreadPoolExecutor>("CPUThreadPoolExecutor"); currentThreadTestDisabled<CPUThreadPoolExecutor>("CPU-ExecutorName");
} }
TEST(ThreadPoolExecutorTest, IOCurrentThreadExecutor) { TEST(ThreadPoolExecutorTest, IOCurrentThreadExecutor) {
currentThreadTest<IOThreadPoolExecutor>("EventBase"); currentThreadTest<IOThreadPoolExecutor>("IO-ExecutorName");
currentThreadTestDisabled<IOThreadPoolExecutor>("EventBase"); currentThreadTestDisabled<IOThreadPoolExecutor>("IO-ExecutorName");
} }
TEST(ThreadPoolExecutorTest, EDFCurrentThreadExecutor) { TEST(ThreadPoolExecutorTest, EDFCurrentThreadExecutor) {
currentThreadTest<EDFThreadPoolExecutor>("EDFThreadPoolExecutor"); currentThreadTest<EDFThreadPoolExecutor>("EDF-ExecutorName");
currentThreadTestDisabled<EDFThreadPoolExecutor>("EDFThreadPoolExecutor"); currentThreadTestDisabled<EDFThreadPoolExecutor>("EDF-ExecutorName");
} }
...@@ -37,7 +37,6 @@ ...@@ -37,7 +37,6 @@
#include <folly/system/ThreadName.h> #include <folly/system/ThreadName.h>
namespace { namespace {
constexpr folly::StringPiece executorName = "EventBase";
class EventBaseBackend : public folly::EventBaseBackendBase { class EventBaseBackend : public folly::EventBaseBackendBase {
public: public:
...@@ -279,7 +278,8 @@ void EventBase::waitUntilRunning() { ...@@ -279,7 +278,8 @@ void EventBase::waitUntilRunning() {
// enters the event_base loop -- will only exit when forced to // enters the event_base loop -- will only exit when forced to
bool EventBase::loop() { bool EventBase::loop() {
ExecutorBlockingGuard guard{ExecutorBlockingGuard::TrackTag{}, executorName}; // Enforce blocking tracking and if we have a name override any previous one
ExecutorBlockingGuard guard{ExecutorBlockingGuard::TrackTag{}, this, name_};
return loopBody(); return loopBody();
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment