[PATCH] D24991: Inline hot functions in libcxx shared_ptr implementation.

[Eric Fiselier via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D24991#565715, @mclow.lists wrote:

> How does this play with existing binaries?  Applications that expect these functions to exist in the dylib?


This patch is majorly ABI breaking, although we could probably find a formulation that wasn't.

@hxy9243 wrote:

>   which gives potential optimization opportunities and performance benefits.


Please provide benchmark tests which demonstrate that these benefits are concrete and not just "potential".  Moving methods out of the dylib is no easy task so I would like hard evidence that it's worth while.



================
Comment at: libcxx/include/memory:3793
 
+namespace
+{
----------------
Anonymous namespaces are a C++11 feature and this is C++03 code. 


================
Comment at: libcxx/include/memory:3798
+// See https://llvm.org/bugs/show_bug.cgi?id=22803
+template <class T> __attribute__((always_inline))
+inline _LIBCPP_INLINE_VISIBILITY T
----------------
* `T` and `increment` need to be reserved names.
* Never use `__attribute__((always_inline))` directly, that's why we have visibility macros.


================
Comment at: libcxx/include/memory:3802
+{
+  return __libcpp_atomic_add(&t, 1, _AO_Relaxed);
+}
----------------
Why add `increment` and `decrement` at all? Just manually inline `__libcpp_atomic_add` at the callsites.


================
Comment at: libcxx/include/memory:3818
     virtual ~bad_weak_ptr() _NOEXCEPT;
-    virtual const char* what() const  _NOEXCEPT;
+    virtual const char* what() const  _NOEXCEPT {
+      return "bad_weak_ptr";
----------------
Why would you  want to inline this?


Repository:
  rL LLVM

https://reviews.llvm.org/D24991