Hi Jonathon and Srđan,
On Fri, 2019-08-16 at 14:24 -0500, Jonathon Anderson wrote:
> For parallel applications that need the information in the DIEs, the
> Dwarf_Abbrev hash table et al. become a massive data race. This fixes
> that by:
>
> 1. Adding atomics & locks to the hash table to manage concurrency
> (lib/dynamicsizehash_concurrent.{c,h})
> 2. Adding a lock & array structure to the memory manager (pseudo-TLS)
> (libdwP.h, libdw_alloc.c)
> 3. Adding extra configure options for Helgrind/DRD annotations
> (configure.ac)
> 4. Including "stdatomic.h" from FreeBSD, to support C11-style atomics.
> (lib/stdatomic.h)
This looks like really nice work. Thanks!
I am splitting review in some smaller parts if you don't mind.
Simply because it is large and I cannot keep everything in my head at
once :) But here some initial overall comments.
> Notes:
> - GCC >= 4.9 provides <stdatomic.h> natively; for those versions
> lib/stdatomic.h could be removed or disabled. We can also rewrite the
> file if the copyright becomes an issue.
If the compiler provides stdatomic.h then I think it would be good to
use that instead of our own implementation. The copyright isn't a
problem. But do you have a reference/URL to the upstream version? I
like to add that somewhere, so we can sync with it in the future. I see
various commented out parts. Was that already upstream? Should we just
remove those parts?
> - Currently the concurrent hash table is always enabled,
> performance-wise there is no known difference between it
> and the non-concurrent version.
> This can be changed to toggle with --enable-thread-safety
> if preferred.
I would prefer it always enabled, unless there is a massive slowdown of
the single-threaded case. The problem with --enable-thread-safety is
that it is a) known broken (sigh) and b) it basically introduces two
separate libraries that behave subtly differently. I would very much
like to get rid of --enable-thread-safety by fixing the broken locking
and simply making it the default.
> - Another implementation of #2 above might use dynamic TLS
> (pthread_key_*),
> we chose this implementation to reduce the overall complexity.
Are there any other trade-offs?
Thanks,
Mark
