[libc++] Add ABI flag to make __tree nodes more compact

[philnik777]

Currently, the __tree_node_base consists of three pointers and a bool. If an eight byte aligned object is allocated, this results in seven additional padding bytes. With this ABI flag we instead put the bool into the low bit of one of the pointers, making every node eight bytes smaller. Consider for example string. There every node goes down from 56 to 48 bytes.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/147681.diff

2 Files Affected:

• (modified) libcxx/include/__configuration/abi.h (+3)
• (modified) libcxx/include/__tree (+38)

diff --git a/libcxx/include/__configuration/abi.h b/libcxx/include/__configuration/abi.h
index a75cd0a675339..759687914be91 100644
--- a/libcxx/include/__configuration/abi.h
+++ b/libcxx/include/__configuration/abi.h
@@ -75,6 +75,7 @@
 #  define _LIBCPP_ABI_OPTIMIZED_FUNCTION
 #  define _LIBCPP_ABI_REGEX_CONSTANTS_NONZERO
 #  define _LIBCPP_ABI_STRING_OPTIMIZED_EXTERNAL_INSTANTIATION
+#  define _LIBCPP_ABI_TREE_POINTER_INT_PAIR
 #  define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
 #  define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
 #  define _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
@@ -98,6 +99,8 @@
 #  endif
 #endif
 
+#define _LIBCPP_ABI_TREE_POINTER_INT_PAIR
+
 // We had some bugs where we use [[no_unique_address]] together with construct_at,
 // which causes UB as the call on construct_at could write to overlapping subobjects
 //
diff --git a/libcxx/include/__tree b/libcxx/include/__tree
index 6b025bcf3496d..3ead4c286f47f 100644
--- a/libcxx/include/__tree
+++ b/libcxx/include/__tree
@@ -41,6 +41,7 @@
 #include <__utility/forward.h>
 #include <__utility/move.h>
 #include <__utility/pair.h>
+#include <__utility/pointer_int_pair.h>
 #include <__utility/swap.h>
 #include <limits>
 
@@ -593,6 +594,43 @@ public:
   __tree_node_base& operator=(__tree_node_base const&) = delete;
 };
 
+#ifdef _LIBCPP_ABI_TREE_POINTER_INT_PAIR
+template <>
+class __tree_node_base<void*> : public __tree_end_node<__tree_node_base<void*>*> {
+public:
+  using pointer                            = __tree_node_base<void*>*;
+  using __end_node_pointer _LIBCPP_NODEBUG = __tree_end_node<__tree_node_base<void*>*>*;
+
+  pointer __right_;
+
+private:
+  using __pair_t = __pointer_int_pair<__end_node_pointer, bool, __integer_width(1)>;
+
+  __pair_t __parent_and_color_;
+
+public:
+  _LIBCPP_HIDE_FROM_ABI pointer __parent_unsafe() const {
+    return static_cast<pointer>(__parent_and_color_.__get_ptr());
+  }
+
+  _LIBCPP_HIDE_FROM_ABI void __set_parent(pointer __ptr) { __set_parent(static_cast<__end_node_pointer>(__ptr)); }
+
+  _LIBCPP_HIDE_FROM_ABI void __set_parent(__end_node_pointer __ptr) {
+    __parent_and_color_ = __pair_t(__ptr, __parent_and_color_.__get_value());
+  }
+
+  _LIBCPP_HIDE_FROM_ABI __end_node_pointer __get_parent() const { return __parent_and_color_.__get_ptr(); }
+  _LIBCPP_HIDE_FROM_ABI __tree_color __get_color() const {
+    return static_cast<__tree_color>(__parent_and_color_.__get_value());
+  }
+  _LIBCPP_HIDE_FROM_ABI void __set_color(__tree_color __color) {
+    __parent_and_color_ = __pair_t(__parent_and_color_.__get_ptr(), __color == __tree_color::__black);
+  }
+};
+#endif // _LIBCPP_ABI_TREE_POINTER_INT_PAIR
+
+static_assert(sizeof(__tree_node_base<void*>) == 24);
+
 template <class _Tp, class _VoidPtr>
 class _LIBCPP_STANDALONE_DEBUG __tree_node : public __tree_node_base<_VoidPtr> {
 public:

[ldionne]

I think the base specialization should use __pointer_int_pair itself, that way if a fancy pointer has free bits (which is likely), we'd be able to reuse those as well. That would require the __pointer_int_pair to have a protocol for the 3rd party fancy pointer to advertise that.

A bit like this:

template <class _VoidPtr>
class _LIBCPP_STANDALONE_DEBUG
__tree_node_base : public __tree_end_node<__rebind_pointer_t<_VoidPtr, __tree_node_base<_VoidPtr> > > {
public:
  using pointer                            = __rebind_pointer_t<_VoidPtr, __tree_node_base>;
  using __end_node_pointer _LIBCPP_NODEBUG = __rebind_pointer_t<_VoidPtr, __tree_end_node<pointer> >;
  pointer __right_;
private:

#if _LIBCPP_ABI_POINTER_INT_PAIR_STUFF
  using __pair_t = __pointer_int_pair<__end_node_pointer, bool, __integer_width(1)>;
  __pair_t __parent_and_color_;
#else
  __end_node_pointer __parent_;
  __tree_color __color_;
#endif

public:
  _LIBCPP_HIDE_FROM_ABI pointer __parent_unsafe() const { return static_cast<pointer>(__parent_); }
  _LIBCPP_HIDE_FROM_ABI void __set_parent(pointer __p) { __parent_ = static_cast<__end_node_pointer>(__p); }
  _LIBCPP_HIDE_FROM_ABI void __set_parent(__end_node_pointer __p) { __parent_ = __p; }
  _LIBCPP_HIDE_FROM_ABI __end_node_pointer __get_parent() const { return __parent_; }
  _LIBCPP_HIDE_FROM_ABI __tree_color __get_color() const { return __color_; }
  _LIBCPP_HIDE_FROM_ABI void __set_color(__tree_color __color) { __color_ = __color; }
  ~__tree_node_base()                                  = delete;
  __tree_node_base(__tree_node_base const&)            = delete;
  __tree_node_base& operator=(__tree_node_base const&) = delete;
};

[philnik777]

I don't think that's a good idea. We don't have any way to let a fancy pointer tell us that it has free bits available, so that argument seems rather moot. If we ever have that we'll want to do a sweep of the library to check whether it makes sense anywhere else anyways. It would also require us to allow __pointer_int_pair to be larger than the pointer type, which would most likely hide programming errors.

[EricWF]

Could you please add some description about why this is an improvement and the validation done test it?

[ldionne]

LGTM but let's A/B measure this to see whether there is a visible impact. I'm especially looking for a regression caused by more expensive pointer chasing since we have to "decode" the pointer now. If we don't see issues with this, I think I'd be OK with making this the new "de facto" ABI for v2 unconditionally.

Also, this obviously needs pointer_int_pair to land.

[ldionne]

Let's add some documentation for this. Also (or only?) in the .rst docs?