-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Reapply "[libc++] Avoid constructing additional objects when using map::at" (#160738) #161485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…p::at" (llvm#160738) This reverts commit b86aaac. The issue in LLVM has been fixed now.
@llvm/pr-subscribers-libcxx Author: Nikolas Klauser (philnik777) ChangesThis reverts commit b86aaac. The issue in LLVM has been fixed now. Patch is 20.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/161485.diff 13 Files Affected:
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index e050362abb658..ddace8bf8c728 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -839,6 +839,7 @@ set(files
__type_traits/is_floating_point.h
__type_traits/is_function.h
__type_traits/is_fundamental.h
+ __type_traits/is_generic_transparent_comparator.h
__type_traits/is_implicit_lifetime.h
__type_traits/is_implicitly_default_constructible.h
__type_traits/is_integral.h
@@ -881,6 +882,7 @@ set(files
__type_traits/make_32_64_or_128_bit.h
__type_traits/make_const_lvalue_ref.h
__type_traits/make_signed.h
+ __type_traits/make_transparent.h
__type_traits/make_unsigned.h
__type_traits/maybe_const.h
__type_traits/nat.h
diff --git a/libcxx/include/__algorithm/comp.h b/libcxx/include/__algorithm/comp.h
index ab3c598418828..38e2fb9f5e744 100644
--- a/libcxx/include/__algorithm/comp.h
+++ b/libcxx/include/__algorithm/comp.h
@@ -11,6 +11,7 @@
#include <__config>
#include <__type_traits/desugars_to.h>
+#include <__type_traits/is_generic_transparent_comparator.h>
#include <__type_traits/is_integral.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -48,6 +49,9 @@ inline const bool __desugars_to_v<__less_tag, __less<>, _Tp, _Tp> = true;
template <class _Tp>
inline const bool __desugars_to_v<__totally_ordered_less_tag, __less<>, _Tp, _Tp> = is_integral<_Tp>::value;
+template <>
+inline const bool __is_generic_transparent_comparator_v<__less<> > = true;
+
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___ALGORITHM_COMP_H
diff --git a/libcxx/include/__functional/is_transparent.h b/libcxx/include/__functional/is_transparent.h
index 567df1a662f54..c2c6fbce2465b 100644
--- a/libcxx/include/__functional/is_transparent.h
+++ b/libcxx/include/__functional/is_transparent.h
@@ -29,6 +29,14 @@ inline const bool __is_transparent_v<_Tp, _Key, __void_t<typename _Tp::is_transp
#endif
+// Two types are considered transparently comparable if `comparator(key, arg)` is equivalent to `comparator(key,
+// <implicit cast to KeyT>(arg))`.
+//
+// This is different from `__is_transparent_v`, which is only a property of the comparator and doesn't provide
+// additional semantic guarantees.
+template <class _Comparator, class _KeyT, class _Arg, class = void>
+inline const bool __is_transparently_comparable_v = false;
+
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___FUNCTIONAL_IS_TRANSPARENT
diff --git a/libcxx/include/__functional/operations.h b/libcxx/include/__functional/operations.h
index 7b0ea11db5844..7f315ca851c08 100644
--- a/libcxx/include/__functional/operations.h
+++ b/libcxx/include/__functional/operations.h
@@ -15,7 +15,9 @@
#include <__functional/unary_function.h>
#include <__fwd/functional.h>
#include <__type_traits/desugars_to.h>
+#include <__type_traits/is_generic_transparent_comparator.h>
#include <__type_traits/is_integral.h>
+#include <__type_traits/make_transparent.h>
#include <__utility/forward.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -377,6 +379,14 @@ struct less<void> {
typedef void is_transparent;
};
+template <class _Tp>
+struct __make_transparent<less<_Tp> > {
+ using type _LIBCPP_NODEBUG = less<>;
+};
+
+template <>
+inline const bool __is_generic_transparent_comparator_v<less<>> = true;
+
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__less_tag, less<>, _Tp, _Up> = true;
@@ -466,6 +476,14 @@ struct greater<void> {
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__greater_tag, greater<>, _Tp, _Up> = true;
+
+template <class _Tp>
+struct __make_transparent<greater<_Tp>> {
+ using type _LIBCPP_NODEBUG = greater<>;
+};
+
+template <>
+inline const bool __is_generic_transparent_comparator_v<greater<>> = true;
#endif
// Logical operations
diff --git a/libcxx/include/__functional/ranges_operations.h b/libcxx/include/__functional/ranges_operations.h
index df95843e7c9af..dc9da061af264 100644
--- a/libcxx/include/__functional/ranges_operations.h
+++ b/libcxx/include/__functional/ranges_operations.h
@@ -14,6 +14,7 @@
#include <__concepts/totally_ordered.h>
#include <__config>
#include <__type_traits/desugars_to.h>
+#include <__type_traits/is_generic_transparent_comparator.h>
#include <__utility/forward.h>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
@@ -108,6 +109,12 @@ inline const bool __desugars_to_v<__less_tag, ranges::less, _Tp, _Up> = true;
template <class _Tp, class _Up>
inline const bool __desugars_to_v<__greater_tag, ranges::greater, _Tp, _Up> = true;
+template <>
+inline const bool __is_generic_transparent_comparator_v<ranges::less> = true;
+
+template <>
+inline const bool __is_generic_transparent_comparator_v<ranges::greater> = true;
+
#endif // _LIBCPP_STD_VER >= 20
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 61c910c52c536..ef960d481cb7b 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -34,6 +34,7 @@
#include <__type_traits/is_same.h>
#include <__type_traits/is_specialization.h>
#include <__type_traits/is_swappable.h>
+#include <__type_traits/make_transparent.h>
#include <__type_traits/remove_const.h>
#include <__utility/forward.h>
#include <__utility/lazy_synth_three_way_comparator.h>
@@ -1749,7 +1750,8 @@ __tree<_Tp, _Compare, _Allocator>::__find_equal(const _Key& __v) {
}
__node_base_pointer* __node_ptr = __root_ptr();
- auto __comp = __lazy_synth_three_way_comparator<_Compare, _Key, value_type>(value_comp());
+ auto&& __transparent = std::__as_transparent(value_comp());
+ auto __comp = __lazy_synth_three_way_comparator<__make_transparent_t<_Compare>, _Key, value_type>(__transparent);
while (true) {
auto __comp_res = __comp(__v, __nd->__get_value());
diff --git a/libcxx/include/__type_traits/is_generic_transparent_comparator.h b/libcxx/include/__type_traits/is_generic_transparent_comparator.h
new file mode 100644
index 0000000000000..fd02c0b0423d1
--- /dev/null
+++ b/libcxx/include/__type_traits/is_generic_transparent_comparator.h
@@ -0,0 +1,30 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
+#define _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
+
+#include <__config>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// This traits returns true if the given _Comparator is known to accept any two types for compaison. This is separate
+// from `__is_transparent_v`, since that only enables overloads of specific functions, but doesn't give any semantic
+// guarantees. This trait guarantess that the comparator simply calls the appropriate comparison functions for any two
+// types.
+
+template <class _Comparator>
+inline const bool __is_generic_transparent_comparator_v = false;
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_IS_GENERIC_TRANSPARENT_COMPARATOR_H
diff --git a/libcxx/include/__type_traits/make_transparent.h b/libcxx/include/__type_traits/make_transparent.h
new file mode 100644
index 0000000000000..4d3207a807fa7
--- /dev/null
+++ b/libcxx/include/__type_traits/make_transparent.h
@@ -0,0 +1,48 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
+#define _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
+
+#include <__config>
+#include <__type_traits/enable_if.h>
+#include <__type_traits/is_empty.h>
+#include <__type_traits/is_same.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+# pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// __make_transparent tries to create a transparent comparator from its non-transparent counterpart, e.g. obtain
+// `less<>` from `less<T>`. This is useful in cases where conversions can be avoided (e.g. a string literal to a
+// std::string).
+
+template <class _Comparator>
+struct __make_transparent {
+ using type _LIBCPP_NODEBUG = _Comparator;
+};
+
+template <class _Comparator>
+using __make_transparent_t _LIBCPP_NODEBUG = typename __make_transparent<_Comparator>::type;
+
+template <class _Comparator, __enable_if_t<is_same<_Comparator, __make_transparent_t<_Comparator> >::value, int> = 0>
+_LIBCPP_HIDE_FROM_ABI _Comparator& __as_transparent(_Comparator& __comp) {
+ return __comp;
+}
+
+template <class _Comparator, __enable_if_t<!is_same<_Comparator, __make_transparent_t<_Comparator> >::value, int> = 0>
+_LIBCPP_HIDE_FROM_ABI __make_transparent_t<_Comparator> __as_transparent(_Comparator&) {
+ static_assert(is_empty<_Comparator>::value);
+ return __make_transparent_t<_Comparator>();
+}
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_MAKE_TRANSPARENT_H
diff --git a/libcxx/include/map b/libcxx/include/map
index a63dfec910aae..035f913bd3497 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -600,7 +600,10 @@ erase_if(multimap<Key, T, Compare, Allocator>& c, Predicate pred); // C++20
# include <__ranges/from_range.h>
# include <__tree>
# include <__type_traits/container_traits.h>
+# include <__type_traits/desugars_to.h>
# include <__type_traits/is_allocator.h>
+# include <__type_traits/is_convertible.h>
+# include <__type_traits/make_transparent.h>
# include <__type_traits/remove_const.h>
# include <__type_traits/type_identity.h>
# include <__utility/forward.h>
@@ -666,6 +669,11 @@ public:
# endif
};
+template <class _Key, class _MapValueT, class _Compare>
+struct __make_transparent<__map_value_compare<_Key, _MapValueT, _Compare> > {
+ using type _LIBCPP_NODEBUG = __map_value_compare<_Key, _MapValueT, __make_transparent_t<_Compare> >;
+};
+
# if _LIBCPP_STD_VER >= 14
template <class _MapValueT, class _Key, class _Compare>
struct __lazy_synth_three_way_comparator<__map_value_compare<_Key, _MapValueT, _Compare>, _MapValueT, _MapValueT> {
@@ -1048,6 +1056,24 @@ public:
_LIBCPP_HIDE_FROM_ABI mapped_type& operator[](key_type&& __k);
# endif
+ template <class _Arg,
+ __enable_if_t<__is_transparently_comparable_v<_Compare, key_type, __remove_cvref_t<_Arg> >, int> = 0>
+ _LIBCPP_HIDE_FROM_ABI mapped_type& at(_Arg&& __arg) {
+ auto [_, __child] = __tree_.__find_equal(__arg);
+ if (__child == nullptr)
+ std::__throw_out_of_range("map::at: key not found");
+ return static_cast<__node_pointer>(__child)->__get_value().second;
+ }
+
+ template <class _Arg,
+ __enable_if_t<__is_transparently_comparable_v<_Compare, key_type, __remove_cvref_t<_Arg> >, int> = 0>
+ _LIBCPP_HIDE_FROM_ABI const mapped_type& at(_Arg&& __arg) const {
+ auto [_, __child] = __tree_.__find_equal(__arg);
+ if (__child == nullptr)
+ std::__throw_out_of_range("map::at: key not found");
+ return static_cast<__node_pointer>(__child)->__get_value().second;
+ }
+
_LIBCPP_HIDE_FROM_ABI mapped_type& at(const key_type& __k);
_LIBCPP_HIDE_FROM_ABI const mapped_type& at(const key_type& __k) const;
@@ -1242,11 +1268,15 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator find(const key_type& __k) { return __tree_.find(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator find(const key_type& __k) const { return __tree_.find(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator find(const _K2& __k) {
return __tree_.find(__k);
}
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator find(const _K2& __k) const {
return __tree_.find(__k);
}
@@ -1262,7 +1292,9 @@ public:
# if _LIBCPP_STD_VER >= 20
_LIBCPP_HIDE_FROM_ABI bool contains(const key_type& __k) const { return find(__k) != end(); }
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI bool contains(const _K2& __k) const {
return find(__k) != end();
}
@@ -1271,12 +1303,16 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const key_type& __k) { return __tree_.lower_bound(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const key_type& __k) const { return __tree_.lower_bound(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator lower_bound(const _K2& __k) {
return __tree_.lower_bound(__k);
}
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator lower_bound(const _K2& __k) const {
return __tree_.lower_bound(__k);
}
@@ -1285,11 +1321,15 @@ public:
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const key_type& __k) { return __tree_.upper_bound(__k); }
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const key_type& __k) const { return __tree_.upper_bound(__k); }
# if _LIBCPP_STD_VER >= 14
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI iterator upper_bound(const _K2& __k) {
return __tree_.upper_bound(__k);
}
- template <typename _K2, enable_if_t<__is_transparent_v<_Compare, _K2>, int> = 0>
+ template <typename _K2,
+ enable_if_t<__is_transparent_v<_Compare, _K2> || __is_transparently_comparable_v<_Compare, key_type, _K2>,
+ int> = 0>
_LIBCPP_HIDE_FROM_ABI const_iterator upper_bound(const _K2& __k) const {
return __tree_.upper_bound(__k);
}
diff --git a/libcxx/include/module.modulemap.in b/libcxx/include/module.modulemap.in
index 93d43f8d7e195..894093b409e11 100644
--- a/libcxx/include/module.modulemap.in
+++ b/libcxx/include/module.modulemap.in
@@ -200,6 +200,7 @@ module std_core [system] {
header "__type_traits/is_fundamental.h"
export std_core.type_traits.integral_constant
}
+ module is_generic_transparent_comparator { header "__type_traits/is_generic_transparent_comparator.h" }
module is_implicit_lifetime {
header "__type_traits/is_implicit_lifetime.h"
export std_core.type_traits.integral_constant
@@ -353,6 +354,7 @@ module std_core [system] {
module make_32_64_or_128_bit { header "__type_traits/make_32_64_or_128_bit.h" }
module make_const_lvalue_ref { header "__type_traits/make_const_lvalue_ref.h" }
module make_signed { header "__type_traits/make_signed.h" }
+ module make_transparent { header "__type_traits/make_transparent.h" }
module make_unsigned { header "__type_traits/make_unsigned.h" }
module maybe_const { header "__type_traits/maybe_const.h" }
module nat { header "__type_traits/nat.h" }
diff --git a/libcxx/include/string b/libcxx/include/string
index cfd6861e5c9c2..dc562e0207630 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -600,6 +600,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
# include <__debug_utils/sanitizers.h>
# include <__format/enable_insertable.h>
# include <__functional/hash.h>
+# include <__functional/is_transparent.h>
# include <__functional/unary_function.h>
# include <__fwd/string.h>
# include <__iterator/bounded_iter.h>
@@ -628,6 +629,7 @@ basic_string<char32_t> operator""s( const char32_t *str, size_t len );
# include <__type_traits/is_allocator.h>
# include <__type_traits/is_array.h>
# include <__type_traits/is_convertible.h>
+# include <__type_traits/is_generic_transparent_comparator.h>
# include <__type_traits/is_nothrow_assignable.h>
# include <__type_traits/is_nothrow_constructible.h>
# include <__type_traits/is_replaceable.h>
@@ -2567,6 +2569,20 @@ struct __default_three_way_comparator<basic_string<_CharT, _Traits, _Alloc>, bas
};
# endif
+template <class _Comparator, class _CharT, class _Traits, class _Alloc>
+inline const bool __is_transparently_comparable_v<_Comparator,
+ basic_string<_CharT, _Traits, _Alloc>,
+ const _CharT*,
+ __enable_if_t<__is_generic_transparent_comparator_v<_Comparator> > > =
+ true;
+
+template <class _Comparator, class _CharT, class _Traits, class _Alloc, size_t _Np>
+inline const bool __is_transparently_comparable_v<_Comparator,
+ basic_string<_CharT, _Traits, _Alloc>,
+ _CharT[_Np],
+ __enable_if_t<__is_generic_transparent_comparator_v<_Comparator> > > =
+ true;
+
# if _LIBCPP_STD_VER >= 17
template <class _InputIterator,
class _CharT = __iter_value_type<_InputIterator>,
diff --git a/libcxx/test/benchmarks/containers/associative/map.bench.cpp b/libcxx/test/benchmarks/containers/associative/map.bench.cpp
index bd664dbb56ee7..142229ae64cad 100644
--- a/libcxx/test/benchmarks/containers/associative/map.bench.cpp
+++ b/libcxx/test/benchmarks/containers/associative/map.bench.cpp
@@ -16,6 +16,19 @@
#include "../../GenerateInput.h"
#include "benchmark/benchmark.h"
+static void BM_map_find_string_literal(benchmark::State& state) {
+ std::map<std::string, int> map;
+ map.emplace("Something very very long to show a long string situation", 1);
+ map.emplace("Something Else", 2);
+
+ for (auto _ : state) {
+ benchmark::DoNotOptimize(map);
+ benchmark::DoNotOptimize(map.find("Something very very long to show a long string situation"));
+ }
+}
+
+BENCHMARK(BM_map_find_string_literal);
+
template <class K, class V>
struct support::adapt_operations<std::map<K, V>> {
using ValueType = typename std::map<K, V>::value_type;
diff --git a/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp b/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
index 57adec2d214d4..d670c531910ea 100644
--- a/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
+++ b/libcxx/test/benchmarks/containers/associative/unordered_map.bench.cpp
@@ -15,6 +15,19 @@
#include "../../GenerateInput.h"
#include "benchmark/benchmark.h"
+static void BM_map_find_string_literal(benchmark::State& state) {
+ std::unordered_map<std::string, int> map;
+ map.emplace("Something very very long to show a long string situation", 1);
+ map.emplace("Something Else", 2);
+
+ for (auto _ : state) {
+ benchmark::DoNotOptimize(map);
+ benchmark::DoNotOptimize(map.find("Something very very long to show a long strin...
[truncated]
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/12867 Here is the relevant piece of the build log for the reference
|
…p::at" (llvm#160738) (llvm#161485) This reverts commit b86aaac. The issue in LLVM has been fixed now.
Do you happen to have a link to the LLVM fix? This breaks some of our code – likely our fault, but it'd be convenient to see what other fixes that this required looked like :) |
Looks like #160804 is what fixed things. |
Hm, ours looks different. Here's a reduced repro of ours:
Before this PR, the code built fine. Is that |
(If I copy in |
@nico This specialization looks invalid to me, as the Although it's permitted to specialize |
This reverts commit b86aaac.
The issue in LLVM has been fixed now.