This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Improve performance of __dynamic_cast
AbandonedPublic

Authored by Lancern on Nov 3 2022, 12:08 AM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
Restricted Project
Summary

The original __dynamic_cast implementation does not use the ABI-provided src2dst_offset parameter which helps improve performance on the hot paths. This patch improves the performance on the hot paths in __dynamic_cast by leveraging hints provided by the src2dst_offset parameter. This patch also includes a performance benchmark suite for the __dynamic_cast implementation.

Diff Detail

Event Timeline

Lancern created this revision.Nov 3 2022, 12:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 12:08 AM
Lancern requested review of this revision.Nov 3 2022, 12:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 3 2022, 12:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
nikic edited the summary of this revision. (Show Details)Nov 3 2022, 1:48 AM

Could you provide before and after numbers for the benchmark?

Could you provide before and after numbers for the benchmark?

The following text file contains the benchmark results. The numbers are measured in nanoseconds.

inclyc added a subscriber: inclyc.Nov 4 2022, 4:00 AM
xingxue added a subscriber: xingxue.Nov 4 2022, 8:57 AM
junaire added a subscriber: junaire.Nov 5 2022, 6:49 AM

Could you provide before and after numbers for the benchmark?

The following text file contains the benchmark results. The numbers are measured in nanoseconds.

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.

libcxx/benchmarks/libcxxabi/dynamic_cast.bench.cpp
1

Please add the license comment.

libcxxabi/src/private_typeinfo.cpp
626

Please remove this comment.

660

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?

685

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.

Lancern updated this revision to Diff 473646.Nov 7 2022, 6:05 AM

Resolve the reviewed issues posted at D137315#3910662

Lancern marked 4 inline comments as done.Nov 7 2022, 6:12 AM

I instrumented private_typeinfo.cpp with gcov and get its coverage during unit tests. The coverage shows that all new branches introduced in this patch series are covered by existing unit tests.

Below is the gcov report for private_typeinfo.cpp if you're interested.

EricWF added a subscriber: EricWF.Nov 14 2022, 9:08 AM

Do you know why the hint wasn't initially used? Was there a point where compilers didn't actually provide the hints?

Also, do you think there would be a way to split this change into two? One that does the refactoring without changing behavior, and another (hopefully much smaller) change that starts taking advantage of the src2dst_offset hint?

I think it would be much easier to understand the changes here if they were separated from the nice cleanup that was done here.

libcxxabi/src/private_typeinfo.cpp
260

Would it be better to pass this struct in rather than taking a bunch of very similar parameters with the same type as function arguments?

It seems like that would be less bug prone.

649

Seems like it would be a better API to have dyn_cast_get_derived_info to return the info struct rather than taking it as an input parameter.

Do you know why the hint wasn't initially used? Was there a point where compilers didn't actually provide the hints?

I tested some compilers supported by Compiler Explorer and:

  • For GCC, the oldest version (4.1.2) supported by Compiler Explorer provides the hints;
  • Clang starts providing the hints since version 3.3;
  • ICC starts providing the hints since version 19.0.0;
  • For ICX, the oldest version (2021.1.2) supported by Compiler Explorer provides the hints.

MSVC is not tested since it does not generate code compatible with the Itanium ABI.

Also, do you think there would be a way to split this change into two? One that does the refactoring without changing behavior, and another (hopefully much smaller) change that starts taking advantage of the src2dst_offset hint?

Actually this revision contains two commits. The first commit starts taking advantage of the src2dst_offset hint, and the second commit refactors without changing behavior. Looks like Differential only shows the latest commit in the patch series.

Lancern abandoned this revision.Nov 14 2022, 11:42 PM

This revision is abandoned since I screwed it with multiple commits. I re-created 2 new revisions for each of the 2 commits contained in this revision, respectively:

  • D138005 that contains the behavior-changing commit that takes uses of the src2dst_offset hint;
  • D138006 that contains the refactorings without changing behaviors.