Commit 72e91069 authored by Zhengxu Chen's avatar Zhengxu Chen Committed by Facebook GitHub Bot

Make RequestContext rootid value atomic in RequestContext::StaticContext.

Summary:
Currently RequestContext::getRootidsFromAllThreads() can access StaticContext root ids concurrently with RequestContext::setRequestContext() without synchronization guards, which is not safe cross other platforms than x64 and also causing TSAN warnings when used in server contexts.
Therefore we can make it std::atomic and access it with relaxed order. This won't affect performance in major platform like x64 since the loads/stores instructions will be the same.
Also as a side effect this prevents RequestContext::StaticContext from generating copy/move constructor, so we remove an extra copy of root id in StaticContext in setRequestContext path.

Reviewed By: yfeldblum

Differential Revision: D22083558

fbshipit-source-id: ed70e394f1172b07ba00c15f05a370f84265785c
parent 7e9c1601
...@@ -754,11 +754,11 @@ FOLLY_ALWAYS_INLINE ...@@ -754,11 +754,11 @@ FOLLY_ALWAYS_INLINE
/* static */ std::shared_ptr<RequestContext> RequestContext::setContextLock( /* static */ std::shared_ptr<RequestContext> RequestContext::setContextLock(
std::shared_ptr<RequestContext>& newCtx, std::shared_ptr<RequestContext>& newCtx,
StaticContext& staticCtx) { StaticContext& staticCtx) {
auto curCtx = staticCtx; auto curCtx = staticCtx.first;
if (newCtx && curCtx.first) { if (newCtx && curCtx) {
// Only call set/unset for all request data that differs // Only call set/unset for all request data that differs
auto ret = folly::acquireLocked( auto ret = folly::acquireLocked(
as_const(newCtx->state_), as_const(curCtx.first->state_)); as_const(newCtx->state_), as_const(curCtx->state_));
auto& newLock = std::get<0>(ret); auto& newLock = std::get<0>(ret);
auto& curLock = std::get<1>(ret); auto& curLock = std::get<1>(ret);
auto& newData = newLock->callbackData_; auto& newData = newLock->callbackData_;
...@@ -766,34 +766,34 @@ FOLLY_ALWAYS_INLINE ...@@ -766,34 +766,34 @@ FOLLY_ALWAYS_INLINE
exec_set_difference( exec_set_difference(
curData, newData, [](RequestData* data) { data->onUnset(); }); curData, newData, [](RequestData* data) { data->onUnset(); });
staticCtx.first = newCtx; staticCtx.first = newCtx;
staticCtx.second = newCtx->rootId_; staticCtx.second.store(newCtx->rootId_, std::memory_order_relaxed);
exec_set_difference( exec_set_difference(
newData, curData, [](RequestData* data) { data->onSet(); }); newData, curData, [](RequestData* data) { data->onSet(); });
} else { } else {
if (curCtx.first) { if (curCtx) {
curCtx.first->onUnset(); curCtx->onUnset();
} }
staticCtx.first = newCtx; staticCtx.first = newCtx;
if (newCtx) { if (newCtx) {
staticCtx.second = newCtx->rootId_; staticCtx.second.store(newCtx->rootId_, std::memory_order_relaxed);
newCtx->onSet(); newCtx->onSet();
} else { } else {
staticCtx.second = 0; staticCtx.second.store(0, std::memory_order_relaxed);
} }
} }
return curCtx.first; return curCtx;
} }
FOLLY_ALWAYS_INLINE FOLLY_ALWAYS_INLINE
/* static */ std::shared_ptr<RequestContext> RequestContext::setContextHazptr( /* static */ std::shared_ptr<RequestContext> RequestContext::setContextHazptr(
std::shared_ptr<RequestContext>& newCtx, std::shared_ptr<RequestContext>& newCtx,
StaticContext& staticCtx) { StaticContext& staticCtx) {
auto curCtx = std::move(staticCtx); auto curCtx = std::move(staticCtx.first);
bool checkCur = curCtx.first && curCtx.first->stateHazptr_.combined(); bool checkCur = curCtx && curCtx->stateHazptr_.combined();
bool checkNew = newCtx && newCtx->stateHazptr_.combined(); bool checkNew = newCtx && newCtx->stateHazptr_.combined();
if (checkCur && checkNew) { if (checkCur && checkNew) {
hazptr_array<2> h; hazptr_array<2> h;
auto curc = h[0].get_protected(curCtx.first->stateHazptr_.combined_); auto curc = h[0].get_protected(curCtx->stateHazptr_.combined_);
auto newc = h[1].get_protected(newCtx->stateHazptr_.combined_); auto newc = h[1].get_protected(newCtx->stateHazptr_.combined_);
auto& curcb = curc->callbackData_; auto& curcb = curc->callbackData_;
auto& newcb = newc->callbackData_; auto& newcb = newc->callbackData_;
...@@ -805,7 +805,7 @@ FOLLY_ALWAYS_INLINE ...@@ -805,7 +805,7 @@ FOLLY_ALWAYS_INLINE
} }
} }
staticCtx.first = std::move(newCtx); staticCtx.first = std::move(newCtx);
staticCtx.second = staticCtx.first->rootId_; staticCtx.second.store(staticCtx.first->rootId_, std::memory_order_relaxed);
for (auto it = newcb.begin(); it != newcb.end(); ++it) { for (auto it = newcb.begin(); it != newcb.end(); ++it) {
DCHECK(it.key()); DCHECK(it.key());
auto data = it.key(); auto data = it.key();
...@@ -814,18 +814,19 @@ FOLLY_ALWAYS_INLINE ...@@ -814,18 +814,19 @@ FOLLY_ALWAYS_INLINE
} }
} }
} else { } else {
if (curCtx.first) { if (curCtx) {
curCtx.first->stateHazptr_.onUnset(); curCtx->stateHazptr_.onUnset();
} }
staticCtx.first = std::move(newCtx); staticCtx.first = std::move(newCtx);
if (staticCtx.first) { if (staticCtx.first) {
staticCtx.first->stateHazptr_.onSet(); staticCtx.first->stateHazptr_.onSet();
staticCtx.second = staticCtx.first->rootId_; staticCtx.second.store(
staticCtx.first->rootId_, std::memory_order_relaxed);
} else { } else {
staticCtx.second = 0; staticCtx.second.store(0, std::memory_order_relaxed);
} }
} }
return curCtx.first; return curCtx;
} }
RequestContext::StaticContext& RequestContext::getStaticContext() { RequestContext::StaticContext& RequestContext::getStaticContext() {
...@@ -837,7 +838,9 @@ RequestContext::getRootIdsFromAllThreads() { ...@@ -837,7 +838,9 @@ RequestContext::getRootIdsFromAllThreads() {
std::vector<RootIdInfo> result; std::vector<RootIdInfo> result;
auto accessor = SingletonT::accessAllThreads(); auto accessor = SingletonT::accessAllThreads();
for (auto it = accessor.begin(); it != accessor.end(); ++it) { for (auto it = accessor.begin(); it != accessor.end(); ++it) {
result.push_back({it->second, it.getThreadId(), it.getOSThreadId()}); result.push_back({it->second.load(std::memory_order_relaxed),
it.getThreadId(),
it.getOSThreadId()});
} }
return result; return result;
} }
......
...@@ -292,7 +292,8 @@ class RequestContext { ...@@ -292,7 +292,8 @@ class RequestContext {
RequestContext(const RequestContext& ctx, intptr_t rootid, Tag tag); RequestContext(const RequestContext& ctx, intptr_t rootid, Tag tag);
RequestContext(const RequestContext& ctx, Tag tag); RequestContext(const RequestContext& ctx, Tag tag);
explicit RequestContext(intptr_t rootId); explicit RequestContext(intptr_t rootId);
using StaticContext = std::pair<std::shared_ptr<RequestContext>, intptr_t>; using StaticContext =
std::pair<std::shared_ptr<RequestContext>, std::atomic<intptr_t>>;
private: private:
static StaticContext& getStaticContext(); static StaticContext& getStaticContext();
......
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