[libcxx-commits] [PATCH] D137315: [libc++abi] Improve performance of __dynamic_cast

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a reviewer: ldionne.
philnik added a comment.

In D137315#3904913 <https://reviews.llvm.org/D137315#3904913>, @Lancern wrote:

> In D137315#3904800 <https://reviews.llvm.org/D137315#3904800>, @philnik wrote:
>
>> Could you provide before and after numbers for the benchmark?
>
> The following text file contains the benchmark results. The numbers are measured in nanoseconds.
>
> F25167762: benchmark-results.txt <https://reviews.llvm.org/F25167762>

These numbers look really good! From what I can tell the changes are correct, but someone else should take a look to make sure I didn't miss anything. I'm not super familiar with libc++abi.



================
Comment at: libcxx/benchmarks/libcxxabi/dynamic_cast.bench.cpp:1
+#include <cstddef>
+
----------------
Please add the license comment.


================
Comment at: libcxxabi/src/private_typeinfo.cpp:626
                std::ptrdiff_t src2dst_offset) {
     // Possible future optimization:  Take advantage of src2dst_offset
 
----------------
Please remove this comment.


================
Comment at: libcxxabi/src/private_typeinfo.cpp:660
     {
-        // Using giant short cut.  Add that information to info.
-        info.number_of_dst_type = 1;
-        // Do the  search
-        dynamic_type->search_above_dst(&info, dynamic_ptr, dynamic_ptr, public_path, false);
-#ifdef _LIBCXXABI_FORGIVING_DYNAMIC_CAST
-        // The following if should always be false because we should definitely
-        //   find (static_ptr, static_type), either on a public or private path
-        if (info.path_dst_ptr_to_static_ptr == unknown)
+        // We're downcasting from src_type to the complete object's dynamic
+        //   type. This is a really hot path that can be further optimized
----------------
Please make sure there is sufficient test coverage for the cases you are special-casing for here and if not add tests. The tests are located in `libcxxabi/test`. I also noticed that there is `dynamic_cast_stress.pass.cpp` in there. Could you maybe refactor it to a benchmark and provide numbers for that?


================
Comment at: libcxxabi/src/private_typeinfo.cpp:685
+        {
+            // If src2dst_offset == -3, then:
+            //   src_type is a multiple public base type but never a virtual
----------------
Could you maybe factor the code into a few helper functions? Putting the helpers into an anonymous namespace should result in the same code gen.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137315/new/

https://reviews.llvm.org/D137315