From: Yedidya Feldblum <yfeldblum@fb.com>
Date: Wed, 29 Nov 2017 20:12:01 +0000 (-0800)
Subject: Synchronize coupled caches in folly::threadlocal_detail::StaticMeta
X-Git-Tag: v2017.12.04.00~22
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=73c52b9f7d7dc1db75838b33064a4f7e4a5b2e51;p=folly.git

Synchronize coupled caches in folly::threadlocal_detail::StaticMeta

Summary:
[Folly] Synchronize coupled caches in `folly::threadlocal_detail::StaticMeta`.

The caches should be set together, and only together, because they are coupled. This prevents bugs where one function that sets one cache but not the other cache is inlined into the caller in one module, and another function that reads both caches is inlined into the caller in another module.

Reviewed By: djwatson

Differential Revision: D6435175

fbshipit-source-id: 846c4972b40e525f2c04da6e6609c2ad54f674c0
---

diff --git a/folly/detail/ThreadLocalDetail.h b/folly/detail/ThreadLocalDetail.h
index 735541b5..2f6f10e8 100644
--- a/folly/detail/ThreadLocalDetail.h
+++ b/folly/detail/ThreadLocalDetail.h
@@ -335,45 +335,37 @@ struct StaticMeta : StaticMetaBase {
     return *instance;
   }
 
-#ifdef FOLLY_TLD_USE_FOLLY_TLS
-  // Eliminate as many branches as possible:
-  // One branch on capacityCache, vs. three:
-  // 1) instance() static initializer
-  // 2) getThreadEntry null check
-  // 3) elementsCapacity size check.
-  // 3 will never be true if 1 or 2 are false.
   FOLLY_ALWAYS_INLINE static ElementWrapper& get(EntryID* ent) {
     uint32_t id = ent->getOrInvalid();
-    if (UNLIKELY(capacityCache_ <= id)) {
-      return getSlow(ent);
-    } else {
-      return threadEntryCache_->elements[id];
+#ifdef FOLLY_TLD_USE_FOLLY_TLS
+    static FOLLY_TLS ThreadEntry* threadEntry{};
+    static FOLLY_TLS size_t capacity{};
+    // Eliminate as many branches and as much extra code as possible in the
+    // cached fast path, leaving only one branch here and one indirection below.
+    if (UNLIKELY(capacity <= id)) {
+      getSlowReserveAndCache(ent, id, threadEntry, capacity);
     }
-  }
-
-  static ElementWrapper& getSlow(EntryID* ent) {
-    ElementWrapper& res = instance().getElement(ent);
-    // Cache new capacity
-    capacityCache_ = getThreadEntry()->elementsCapacity;
-    return res;
-  }
 #else
-  static ElementWrapper& get(EntryID* ent) {
-    return instance().getElement(ent);
-  }
+    ThreadEntry* threadEntry{};
+    size_t capacity{};
+    getSlowReserveAndCache(ent, id, threadEntry, capacity);
 #endif
+    return threadEntry->elements[id];
+  }
 
-  ElementWrapper& getElement(EntryID* ent) {
-    ThreadEntry* threadEntry = getThreadEntry();
-    uint32_t id = ent->getOrInvalid();
-    // if id is invalid, it is equal to uint32_t's max value.
-    // x <= max value is always true
+  static void getSlowReserveAndCache(
+      EntryID* ent,
+      uint32_t& id,
+      ThreadEntry*& threadEntry,
+      size_t& capacity) {
+    auto& inst = instance();
+    threadEntry = inst.threadEntry_();
     if (UNLIKELY(threadEntry->elementsCapacity <= id)) {
-      reserve(ent);
+      inst.reserve(ent);
       id = ent->getOrInvalid();
-      assert(threadEntry->elementsCapacity > id);
     }
-    return threadEntry->elements[id];
+    capacity = threadEntry->elementsCapacity;
+    assert(capacity > id);
   }
 
   static ThreadEntry* getThreadEntrySlow() {
@@ -395,17 +387,6 @@ struct StaticMeta : StaticMetaBase {
     return threadEntry;
   }
 
-  inline static ThreadEntry* getThreadEntry() {
-#ifdef FOLLY_TLD_USE_FOLLY_TLS
-    if (UNLIKELY(threadEntryCache_ == nullptr)) {
-      threadEntryCache_ = instance().threadEntry_();
-    }
-    return threadEntryCache_;
-#else
-    return instance().threadEntry_();
-#endif
-  }
-
   static void preFork() {
     instance().lock_.lock();  // Make sure it's created
   }
@@ -417,25 +398,13 @@ struct StaticMeta : StaticMetaBase {
   static void onForkChild() {
     // only the current thread survives
     instance().head_.next = instance().head_.prev = &instance().head_;
-    ThreadEntry* threadEntry = getThreadEntry();
+    ThreadEntry* threadEntry = instance().threadEntry_();
     // If this thread was in the list before the fork, add it back.
     if (threadEntry->elementsCapacity != 0) {
       instance().push_back(threadEntry);
     }
     instance().lock_.unlock();
   }
-
-#ifdef FOLLY_TLD_USE_FOLLY_TLS
-  static FOLLY_TLS ThreadEntry* threadEntryCache_;
-  static FOLLY_TLS size_t capacityCache_;
-#endif
 };
-
-#ifdef FOLLY_TLD_USE_FOLLY_TLS
-template <class Tag, class AccessMode>
-FOLLY_TLS ThreadEntry* StaticMeta<Tag, AccessMode>::threadEntryCache_{nullptr};
-template <class Tag, class AccessMode>
-FOLLY_TLS size_t StaticMeta<Tag, AccessMode>::capacityCache_{0};
-#endif
 } // namespace threadlocal_detail
 } // namespace folly