This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi] Improve performance of __dynamic_cast
ClosedPublic

Authored by Lancern on Nov 14 2022, 11:25 PM.

Details

Reviewers
philnik
ldionne
EricWF
avogelsgesang
Group Reviewers
Restricted Project
Restricted Project
Commits
rGc9d475c937d2: [libc++abi] Improve performance of __dynamic_cast
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 14 2022, 11:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2022, 11:25 PM
Lancern requested review of this revision.Nov 14 2022, 11:25 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 14 2022, 11:25 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Lancern edited the summary of this revision. (Show Details)Nov 14 2022, 11:30 PM

Thanks for looking into this and especially for adding a benchmark! Can you please rebase onto main so that the CI triggers correctly?

Lancern updated this revision to Diff 477688.EditedNov 23 2022, 11:46 PM

Rebase changes onto the main branch so that CI correctly triggers.

Are there any reviewer updates on this revision and the child revision?

philnik accepted this revision.Feb 7 2023, 2:32 AM

LGTM, but you'll need approval from #libc_abi.

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

You are missing the license header.

ldionne accepted this revision.Feb 10 2023, 10:41 AM

LGTM, please add the license header!

This revision is now accepted and ready to land.Feb 10 2023, 10:41 AM
Lancern updated this revision to Diff 496684.Feb 11 2023, 6:42 AM

Add license header to the benchmark source code file.

avogelsgesang accepted this revision.Feb 11 2023, 1:48 PM
avogelsgesang added a subscriber: avogelsgesang.

LGTM

Should there be a Release Note added for this significant performance improvement?

Please make sure you have a green CI run before landing. Adding a release note would make sense -- you can add it in libcxx/.

The libcxx CI fails because:

  • git-clang-format command not found;
  • No CMAKE_C_COMPILER could be found;
  • No CMAKE_CXX_COMPILER could be found.

See https://buildkite.com/llvm-project/libcxx-ci/builds/18924#018640f1-95a6-4945-89f3-854e959d7e42.

Is this a CI configuration issue? How to solve it?

The libcxx CI fails because:

  • git-clang-format command not found;
  • No CMAKE_C_COMPILER could be found;
  • No CMAKE_CXX_COMPILER could be found.

See https://buildkite.com/llvm-project/libcxx-ci/builds/18924#018640f1-95a6-4945-89f3-854e959d7e42.

Is this a CI configuration issue? How to solve it?

You have to rebase on top of main.

Lancern updated this revision to Diff 498838.Feb 20 2023, 6:45 AM

Rebase work onto the latest main.

Nice! The CI is green again :)

@Lancern do you have commit access or do you need someone to commit this on your behalf? Can you still add release notes, pointing out the performance improvements? (If not, I can also propose some release notes for this in a separate patch)

@avogelsgesang I don't have commit access so I think somebody else needs to commit this. And where can I add release notes? I'm not clear on how to add release notes.

The release notes should be added to libcxx/docs/ReleaseNotes.rst as "reStructuredText", in the form of a new bullet point. I guess this point would best fit into the "Improvements and New Features" section. Please make sure to point out in the release notes that this improvement shipped in the libcxxabi and not libcxx

Lancern updated this revision to Diff 499447.Feb 22 2023, 3:54 AM

Update release notes to point out the performance improvements.

Lancern updated this revision to Diff 499448.Feb 22 2023, 4:02 AM

Got rst formatting wrong :( fixed that.

Lancern updated this revision to Diff 499794.Feb 23 2023, 3:34 AM

Rebase work onto the latest main

Lancern updated this revision to Diff 501818.Mar 2 2023, 3:06 AM

Rebase work onto the latest main.

Ping: any updates on landing of this revision?

This revision was automatically updated to reflect the committed changes.