Page MenuHomePhabricator

Cache uwnind frame headers as they are found.
ClosedPublic

Authored by saugustine on Mar 10 2020, 12:09 PM.

Details

Summary

This improves unwind performance quite substantially, and follows
a somewhat similar approach used in libgcc_s as described in the
thread here:

https://gcc.gnu.org/ml/gcc/2005-02/msg00625.html

On certain extremely exception heavy internal tests, the time
drops from about 80 minutes to about five minutes.

Diff Detail

Event Timeline

saugustine created this revision.Mar 10 2020, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 12:09 PM

I have some tests nearing completion, but want to get started with the feedback now.

jgorbe added inline comments.Mar 10 2020, 3:18 PM
libunwind/src/FrameHeaderCache.hpp
46

Do we need any synchronization here if there are multiple threads?

122

This line is redundant, Current has just been initialized to Unused. Did you want to explicitly set Current on each branch? If so, maybe something like this would be clearer?

CacheEntry *Current = nullptr;

if (Unused != nullptr) {
  Current = Unused;
  Unused = Unused->Next;
} else {
  Current = MostRecentlyUsed;
  // ... etc ...
}

in any case, I think changing the condition to Unused != nullptr would be a win, because at least to me it reads more easily as "it there are unused entries...".

saugustine marked 3 inline comments as done.

Update for upstream comments.

saugustine added inline comments.Mar 11 2020, 10:09 AM
libunwind/src/FrameHeaderCache.hpp
46

Good question. We don't because dl_iterate_phdr already holds the load lock and therefore does all the synchronization for us. That isn't obvious at all from reading the code, so adding a comment to that effect.

Add missing comment.

I think it looks good now. My only issue is that it seems to rely on a couple of glibc-specific features: glibc modifying the fields adds and subs in dl_phdr_info when loading/unloading libraries (which this patch uses to know when to invalidate the cache), and dl_iterate_phdr holding a lock (which the patch relies on to avoid races while accessing the cache). What other libc implementations do we support? Do they share these behaviors we rely on here?

I think it looks good now. My only issue is that it seems to rely on a couple of glibc-specific features: glibc modifying the fields adds and subs in dl_phdr_info when loading/unloading libraries (which this patch uses to know when to invalidate the cache), and dl_iterate_phdr holding a lock (which the patch relies on to avoid races while accessing the cache). What other libc implementations do we support? Do they share these behaviors we rely on here?

Freebsd has the dlpi_adds and dlpi_subs are both in freebsd since at least 2012, and musl too. I'm pretty sure the provide equal synchronization guarantees, otherwise they wouldn't be compatible with glibc. And looking at the similar libgcc code, it appears that libgcc also relies on such synchronization.

So I do believe we are OK from that perspective. If it comes to it, we could add a configure-time option, but I'm hoping to avoid that.

Add tests, and minor formatting changes.

Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2020, 3:15 PM

Update test to only run when relevant.

Thanks for doing the extra work of adding a test. Just one final nit.

libunwind/test/frameheadercache_test.pass.cpp
3

I think referring to AddressSpace.hpp for the reasoning behind the ugly #ifdef chain is fine, but I'd add a note about the intention. Suggestion:

// Run the test only in configurations where the caching system is used.
// This #if chain...

Correct the "do not run" cases to match the logic in AddressSpace.hpp.

fix comment

jgorbe accepted this revision.Mar 12 2020, 9:30 AM
This revision is now accepted and ready to land.Mar 12 2020, 9:30 AM
This revision was automatically updated to reflect the committed changes.

Hello @saugustine,

here is failed libunwind test on Aarch64 toolchain builder
http://lab.llvm.org:8011/builders/llvm-clang-win-x-aarch64/builds/5660

  • FAIL: libunwind:: frameheadercache_test.pass.cpp

with a set of similar errors:

C:\buildbot\as-builder-2\llvm-clang-win-x-aarch64\llvm-project\libunwind\test/../src/Registers.hpp:3158:59: error: unused parameter 'regNum' [-Werror,-Wunused-parameter]

inline bool Registers_mips_newabi::validFloatRegister(int regNum) const {

would you take care of it?