Page MenuHomePhabricator

Make ~mutex and ~condition_variable trivial with Apple pthreads
Needs RevisionPublic

Authored by EricWF on Jul 7 2019, 10:47 AM.

Details

Summary

This optimization, as described in PR27658, is safe to perform with Apple pthreads. The implementation of pthread_mutex_destroy does nothing except poison the lock to aid in debugging [1].

If we choose to apply this patch we'll lose some amount of error checking, but I think it's probably worth it given the codegen improvements for global of function local static mutexes.

This same optimization can safely be applied to ~condition_variable, since the destructor doesn't check the return value of the pthread_cond_destroy call. But it looks like we lose more debuggability with condvar than we do with mutex [2][3].

What do you think?

[1] https://opensource.apple.com/source/Libc/Libc-262/pthreads/pthread_mutex.c.auto.html
[2] https://opensource.apple.com/source/Libc/Libc-262/pthreads/pthread_cond.c.auto.html
[3] https://github.com/llvm/llvm-project/blob/master/libcxx/src/condition_variable_destructor.cpp#L40

Diff Detail

Event Timeline

EricWF created this revision.Jul 7 2019, 10:47 AM
EricWF updated this revision to Diff 208298.Jul 7 2019, 10:48 AM
EricWF retitled this revision from Make ~mutex trivial with Apple pthreads to Make ~mutex and ~condition_variable trivial with Apple pthreads.
EricWF set the repository for this revision to rCXX libc++.
EricWF added a subscriber: libcxx-commits.

Looking at the definition of LOCK and UNLOCK in https://opensource.apple.com/source/Libc/Libc-262/pthreads/pthread_internals.h.auto.html, it's not clear to me that we can avoid calling pthread_cond_destroy. I'll ask our OS folks what they think about this.

ldionne requested changes to this revision.Thu, Jul 25, 1:24 PM

Ok, so I spoke to our OS folks and here's what I got.

First of all, we're looking at the wrong sources. The sources we're looking at are about 17 years old (!!!). The most up-to-date public sources for pthread_cond_destroy are https://opensource.apple.com/source/libpthread/libpthread-330.220.2/src/pthread_cond.c.auto.html

Second, it seems like both pthread_cond_destroy and pthread_mutex_destroy can result in the kernel releasing resources it holds internally, so those can't be omitted. It's also the case that synchronization objects must be destroyed (i.e. call pthread_xxx_destroy) before the same address in the process can be used to hold another synchronization object. So, in other words, it seems unsafe to omit these calls. This is unfortunate because of the optimizations this would have enabled, however correctness is clearly more important than performance.

Furthermore, I would strongly encourage being careful with these changes on other platforms. Not because I personally care about such platforms, but I do think that such a tricky optimization should be vetted by folks working on those synchronization primitives from the OS side before anything is done.

I'll be updating the comment to explain that on Apple platforms, pthread_xxx_destroy is NOT a nop, to avoid future confusion.

This revision now requires changes to proceed.Thu, Jul 25, 1:24 PM

I left some comments in r367048