reusable key destructuring logic for implementing emplace

Summary:
This diff extracts the key destructuring logic in F14's emplace
functions into standalone helpers that can be used by other container
types (like sorted_vector_{set,map}).  It also adds better PMR compliance, by
using a stateful allocator to construct a temporary if that is required
to be able to perform the search.

This diff also cleans up some of the testing code in folly/container/test
that had issues if included in multiple places.

Reviewed By: yfeldblum

Differential Revision: D16655131

fbshipit-source-id: 5a6f57ac346d1fd5928e0f737110760b0c518974

CMakeLists.txt

@@ -136,13 +136,15 @@ list(REMOVE_ITEM hfiles
 )
 
 # Explicitly include utility library code from inside
-# folly/test and folly/io/async/test/
+# test subdirs
 list(APPEND files
   ${FOLLY_DIR}/io/async/test/ScopedBoundPort.cpp
   ${FOLLY_DIR}/io/async/test/SocketPair.cpp
   ${FOLLY_DIR}/io/async/test/TimeUtil.cpp
 )
 list(APPEND hfiles
+  ${FOLLY_DIR}/container/test/F14TestUtil.h
+  ${FOLLY_DIR}/container/test/TrackingTypes.h
   ${FOLLY_DIR}/io/async/test/AsyncSSLSocketTest.h
   ${FOLLY_DIR}/io/async/test/AsyncSocketTest.h
   ${FOLLY_DIR}/io/async/test/AsyncSocketTest2.h
@@ -436,11 +438,9 @@ if (BUILD_TESTS)
     ${FOLLY_DIR}/test/SingletonTestStructs.cpp
     ${FOLLY_DIR}/test/SocketAddressTestHelper.cpp
     ${FOLLY_DIR}/test/SocketAddressTestHelper.h
+    ${FOLLY_DIR}/container/test/F14TestUtil.h
+    ${FOLLY_DIR}/container/test/TrackingTypes.h
     ${FOLLY_DIR}/experimental/test/CodingTestUtils.cpp
-    ${FOLLY_DIR}/logging/test/ConfigHelpers.cpp
-    ${FOLLY_DIR}/logging/test/ConfigHelpers.h
-    ${FOLLY_DIR}/logging/test/TestLogHandler.cpp
-    ${FOLLY_DIR}/logging/test/TestLogHandler.h
     ${FOLLY_DIR}/futures/test/TestExecutor.cpp
     ${FOLLY_DIR}/futures/test/TestExecutor.h
     ${FOLLY_DIR}/io/async/test/BlockingSocket.h
@@ -460,6 +460,10 @@ if (BUILD_TESTS)
     ${FOLLY_DIR}/io/async/test/TimeUtil.h
     ${FOLLY_DIR}/io/async/test/UndelayedDestruction.h
     ${FOLLY_DIR}/io/async/test/Util.h
+    ${FOLLY_DIR}/logging/test/ConfigHelpers.cpp
+    ${FOLLY_DIR}/logging/test/ConfigHelpers.h
+    ${FOLLY_DIR}/logging/test/TestLogHandler.cpp
+    ${FOLLY_DIR}/logging/test/TestLogHandler.h
   )
   target_compile_definitions(folly_test_support
     PUBLIC
@@ -502,6 +506,7 @@ if (BUILD_TESTS)
       TEST foreach_test SOURCES ForeachTest.cpp
       TEST merge_test SOURCES MergeTest.cpp
       TEST sparse_byte_set_test SOURCES SparseByteSetTest.cpp
+      TEST util_test SOURCES UtilTest.cpp
 
     DIRECTORY concurrency/test/
       TEST atomic_shared_ptr_test SOURCES AtomicSharedPtrTest.cpp

folly/container/F14Map.h

@@ -30,13 +30,13 @@
 #include <stdexcept>
 
 #include <folly/Traits.h>
-#include <folly/functional/ApplyTuple.h>
 #include <folly/lang/Exception.h>
 #include <folly/lang/SafeAssert.h>
 
 #include <folly/container/F14Map-fwd.h>
 #include <folly/container/detail/F14Policy.h>
 #include <folly/container/detail/F14Table.h>
+#include <folly/container/detail/Util.h>
 
 #if FOLLY_F14_VECTOR_INTRINSICS_AVAILABLE
 
@@ -404,73 +404,20 @@ class F14BasicMap {
   }
 
  private:
-  std::pair<ItemIter, bool> emplaceItem() {
-    // rare but valid
-    return table_.tryEmplaceValue(key_type{});
-  }
-
-  template <typename U1, typename U2>
-  std::pair<ItemIter, bool> emplaceItem(U1&& x, U2&& y) {
-    using K = KeyTypeForEmplace<key_type, hasher, key_equal, U1>;
-    K key(std::forward<U1>(x));
-
-    // TODO(T31574848): piecewise_construct is to work around libstdc++ versions
-    // (e.g., GCC < 6) with no implementation of N4387 ("perfect initialization"
-    // for pairs and tuples).  Otherwise we could just pass key, forwarded key,
-    // and forwarded y to tryEmplaceValue.
-    return table_.tryEmplaceValue(
-        key,
-        std::piecewise_construct,
-        std::forward_as_tuple(std::forward<K>(key)),
-        std::forward_as_tuple(std::forward<U2>(y)));
-  }
-
-  template <typename U1, typename U2>
-  std::pair<ItemIter, bool> emplaceItem(std::pair<U1, U2> const& p) {
-    return emplaceItem(p.first, p.second);
-  }
-
-  template <typename U1, typename U2>
-  std::pair<ItemIter, bool> emplaceItem(std::pair<U1, U2>&& p) {
-    return emplaceItem(std::move(p.first), std::move(p.second));
-  }
-
-  template <typename Arg1, typename... Args2>
-  std::pair<ItemIter, bool> emplaceItem(
-      std::piecewise_construct_t,
-      std::tuple<Arg1>&& first_args,
-      std::tuple<Args2...>&& second_args) {
-    using K = KeyTypeForEmplace<key_type, hasher, key_equal, Arg1>;
-    K key(std::get<0>(std::move(first_args)));
-
-    // Args2&&... holds only references even if the caller gave us a
-    // tuple that directly contains values.
-    return table_.tryEmplaceValue(
-        key,
-        std::piecewise_construct,
-        std::forward_as_tuple(std::forward<K>(key)),
-        std::tuple<Args2&&...>(std::move(second_args)));
-  }
-
-  template <typename... Args1, typename... Args2>
-  std::enable_if_t<sizeof...(Args1) != 1, std::pair<ItemIter, bool>>
-  emplaceItem(
-      std::piecewise_construct_t,
-      std::tuple<Args1...>&& first_args,
-      std::tuple<Args2...>&& second_args) {
-    auto key = make_from_tuple<key_type>(
-        std::tuple<Args1&&...>(std::move(first_args)));
-    return table_.tryEmplaceValue(
-        key,
-        std::piecewise_construct,
-        std::forward_as_tuple(std::move(key)),
-        std::tuple<Args2&&...>(std::move(second_args)));
-  }
+  template <typename Arg>
+  using UsableAsKey =
+      EligibleForHeterogeneousFind<key_type, hasher, key_equal, Arg>;
 
  public:
   template <typename... Args>
   std::pair<iterator, bool> emplace(Args&&... args) {
-    auto rv = emplaceItem(std::forward<Args>(args)...);
+    auto rv = folly::detail::callWithExtractedKey<key_type, UsableAsKey>(
+        table_.alloc(),
+        [&](auto&&... inner) {
+          return table_.tryEmplaceValue(
+              std::forward<decltype(inner)>(inner)...);
+        },
+        std::forward<Args>(args)...);
     return std::make_pair(table_.makeIter(rv.first), rv.second);
   }
 

folly/container/F14Set.h

@@ -34,6 +34,7 @@
 #include <folly/container/F14Set-fwd.h>
 #include <folly/container/detail/F14Policy.h>
 #include <folly/container/detail/F14Table.h>
+#include <folly/container/detail/Util.h>
 
 #if FOLLY_F14_VECTOR_INTRINSICS_AVAILABLE
 
@@ -323,18 +324,21 @@ class F14BasicSet {
     insert(ilist.begin(), ilist.end());
   }
 
+ private:
+  template <typename Arg>
+  using UsableAsKey =
+      EligibleForHeterogeneousFind<key_type, hasher, key_equal, Arg>;
+
+ public:
   template <class... Args>
   std::pair<iterator, bool> emplace(Args&&... args) {
-    using K = KeyTypeForEmplace<key_type, hasher, key_equal, Args...>;
-
-    // If args is a single arg that can be emplaced directly (either
-    // key_type or a heterogeneous find + conversion to key_type) key will
-    // just be a reference to that arg, otherwise this will construct an
-    // intermediate key.
-    K key(std::forward<Args>(args)...);
-
-    auto rv = table_.tryEmplaceValue(key, std::forward<K>(key));
-
+    auto rv = folly::detail::callWithConstructedKey<key_type, UsableAsKey>(
+        table_.alloc(),
+        [&](auto&&... inner) {
+          return table_.tryEmplaceValue(
+              std::forward<decltype(inner)>(inner)...);
+        },
+        std::forward<Args>(args)...);
     return std::make_pair(table_.makeIter(rv.first), rv.second);
   }
 

folly/container/detail/F14Table.h

@@ -39,7 +39,6 @@
 #include <folly/Portability.h>
 #include <folly/ScopeGuard.h>
 #include <folly/Traits.h>
-#include <folly/functional/ApplyTuple.h>
 #include <folly/functional/Invoke.h>
 #include <folly/lang/Align.h>
 #include <folly/lang/Assume.h>
@@ -260,35 +259,6 @@ using EligibleForHeterogeneousInsert = Conjunction<
     EligibleForHeterogeneousFind<TableKey, Hasher, KeyEqual, ArgKey>,
     std::is_constructible<TableKey, ArgKey>>;
 
-template <
-    typename TableKey,
-    typename Hasher,
-    typename KeyEqual,
-    typename KeyArg0OrBool,
-    typename... KeyArgs>
-using KeyTypeForEmplaceHelper = std::conditional_t<
-    sizeof...(KeyArgs) == 1 &&
-        (std::is_same<remove_cvref_t<KeyArg0OrBool>, TableKey>::value ||
-         EligibleForHeterogeneousFind<
-             TableKey,
-             Hasher,
-             KeyEqual,
-             KeyArg0OrBool>::value),
-    KeyArg0OrBool&&,
-    TableKey>;
-
-template <
-    typename TableKey,
-    typename Hasher,
-    typename KeyEqual,
-    typename... KeyArgs>
-using KeyTypeForEmplace = KeyTypeForEmplaceHelper<
-    TableKey,
-    Hasher,
-    KeyEqual,
-    std::tuple_element_t<0, std::tuple<KeyArgs..., bool>>,
-    KeyArgs...>;
-
 ////////////////
 
 template <typename T>

folly/container/detail/Util.h

+/*
+ * Copyright 2017-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+#include <folly/Traits.h>
+#include <folly/functional/ApplyTuple.h>
+
+// Utility functions for container implementors
+
+namespace folly {
+namespace detail {
+
+template <typename KeyType, typename Alloc>
+struct TemporaryEmplaceKey {
+  TemporaryEmplaceKey(TemporaryEmplaceKey const&) = delete;
+  TemporaryEmplaceKey(TemporaryEmplaceKey&&) = delete;
+
+  template <typename... Args>
+  TemporaryEmplaceKey(Alloc& a, std::tuple<Args...>&& args) : alloc_(a) {
+    auto p = &value();
+    apply(
+        [&, p](auto&&... inner) {
+          std::allocator_traits<Alloc>::construct(
+              alloc_, p, std::forward<decltype(inner)>(inner)...);
+        },
+        std::move(args));
+  }
+
+  ~TemporaryEmplaceKey() {
+    std::allocator_traits<Alloc>::destroy(alloc_, &value());
+  }
+
+  KeyType& value() {
+    return *static_cast<KeyType*>(static_cast<void*>(&raw_));
+  }
+
+  Alloc& alloc_;
+  std::aligned_storage_t<sizeof(KeyType), alignof(KeyType)> raw_;
+};
+
+// A map's emplace(args...) function takes arguments that can be used to
+// construct a pair<key_type const, mapped_type>, but that construction
+// only needs to take place if the key is not present in the container.
+// callWithExtractedKey helps to handle this efficiently by looking for a
+// reference to the key within the args list.  If the search is successful
+// then the search can be performed without constructing any temporaries.
+// If the search is not successful then callWithExtractedKey constructs
+// a temporary key_type and a new argument list suitable for constructing
+// the entire value_type if necessary.
+//
+// callWithExtractedKey(a, f, args...) will call f(k, args'...), where
+// k is the key and args'... is an argument list that can be used to
+// construct a pair of key and mapped value.  Note that this means f gets
+// the key twice.
+//
+// In some cases a temporary key must be constructed.  This is accomplished
+// with std::allocator_traits<>::construct, and the temporary will be
+// destroyed with std::allocator_traits<>::destroy.  Using the allocator's
+// construct method reduces unnecessary copies for pmr allocators.
+//
+// callWithExtractedKey supports heterogeneous lookup with the UsableAsKey
+// template parameter.  If a single key argument of type K is found in
+// args... then it will be passed directly to f if it is either KeyType or
+// if UsableAsKey<remove_cvref_t<K>>::value is true.  If you don't care
+// about heterogeneous lookup you can just pass a single-arg template
+// that extends std::false_type.
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename Arg1,
+    typename... Args2,
+    std::enable_if_t<
+        std::is_same<remove_cvref_t<Arg1>, KeyType>::value ||
+            UsableAsKey<remove_cvref_t<Arg1>>::value,
+        int> = 0>
+auto callWithExtractedKey(
+    Alloc&,
+    Func&& f,
+    std::piecewise_construct_t,
+    std::tuple<Arg1>&& first_args,
+    std::tuple<Args2...>&& second_args) {
+  // we found a usable key in the args :)
+  auto const& key = std::get<0>(first_args);
+  return f(
+      key,
+      std::piecewise_construct,
+      std::tuple<Arg1&&>(std::move(first_args)),
+      std::tuple<Args2&&...>(std::move(second_args)));
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename... Args1,
+    typename... Args2>
+auto callWithExtractedKey(
+    Alloc& a,
+    Func&& f,
+    std::piecewise_construct_t,
+    std::tuple<Args1...>&& first_args,
+    std::tuple<Args2...>&& second_args) {
+  // we will need to materialize a temporary key :(
+  TemporaryEmplaceKey<KeyType, Alloc> key(
+      a, std::tuple<Args1&&...>(std::move(first_args)));
+  return f(
+      const_cast<KeyType const&>(key.value()),
+      std::piecewise_construct,
+      std::forward_as_tuple(std::move(key.value())),
+      std::tuple<Args2&&...>(std::move(second_args)));
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func>
+auto callWithExtractedKey(Alloc& a, Func&& f) {
+  return callWithExtractedKey<KeyType, UsableAsKey>(
+      a,
+      std::forward<Func>(f),
+      std::piecewise_construct,
+      std::tuple<>{},
+      std::tuple<>{});
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename U1,
+    typename U2>
+auto callWithExtractedKey(Alloc& a, Func&& f, U1&& x, U2&& y) {
+  return callWithExtractedKey<KeyType, UsableAsKey>(
+      a,
+      std::forward<Func>(f),
+      std::piecewise_construct,
+      std::forward_as_tuple(std::forward<U1>(x)),
+      std::forward_as_tuple(std::forward<U2>(y)));
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename U1,
+    typename U2>
+auto callWithExtractedKey(Alloc& a, Func&& f, std::pair<U1, U2> const& p) {
+  return callWithExtractedKey<KeyType, UsableAsKey>(
+      a,
+      std::forward<Func>(f),
+      std::piecewise_construct,
+      std::forward_as_tuple(p.first),
+      std::forward_as_tuple(p.second));
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename U1,
+    typename U2>
+auto callWithExtractedKey(Alloc& a, Func&& f, std::pair<U1, U2>&& p) {
+  return callWithExtractedKey<KeyType, UsableAsKey>(
+      a,
+      std::forward<Func>(f),
+      std::piecewise_construct,
+      std::forward_as_tuple(std::move(p.first)),
+      std::forward_as_tuple(std::move(p.second)));
+}
+
+// callWithConstructedKey is the set container analogue of
+// callWithExtractedKey
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename Arg,
+    std::enable_if_t<
+        std::is_same<remove_cvref_t<Arg>, KeyType>::value ||
+            UsableAsKey<remove_cvref_t<Arg>>::value,
+        int> = 0>
+auto callWithConstructedKey(Alloc&, Func&& f, Arg&& arg) {
+  // we found a usable key in the args :)
+  auto const& key = arg;
+  return f(key, std::forward<Arg>(arg));
+}
+
+template <
+    typename KeyType,
+    template <typename> class UsableAsKey,
+    typename Alloc,
+    typename Func,
+    typename... Args>
+auto callWithConstructedKey(Alloc& a, Func&& f, Args&&... args) {
+  // we will need to materialize a temporary key :(
+  TemporaryEmplaceKey<KeyType, Alloc> key(
+      a, std::forward_as_tuple(std::forward<Args>(args)...));
+  return f(const_cast<KeyType const&>(key.value()), std::move(key.value()));
+}
+
+} // namespace detail
+} // namespace folly

folly/container/test/F14InterprocessTest.cpp

@@ -29,7 +29,6 @@
 #include <folly/Traits.h>
 #include <folly/container/F14Map.h>
 #include <folly/container/F14Set.h>
-#include <folly/container/test/F14TestUtil.h>
 #include <folly/portability/GTest.h>
 
 using namespace boost::interprocess;