sustrik / libdill

Structured concurrency in C
MIT License
1.68k stars 155 forks source link

Thread autocontext rebase #49

Closed raedwulf closed 7 years ago

raedwulf commented 7 years ago

Cleaned up commits from previous pull request with suggested changes.

raedwulf commented 7 years ago

I'm investigating why clang builds no longer work with OS X.

raedwulf commented 7 years ago

My running theory is that Thread Local Storage behaves differently on OS X. Based on my build log observations, it seems that OS X cannot access Thread Local Storage inside the pthread key destructors. To solve this, I need to pass the pointer to the dill_context through the destructor so that the atexit handlers can cleanup.

raedwulf commented 7 years ago

It appears that this theory was correct. The next commit should work.

There's a couple of FIXME notes that I'll fix soon - these are to remove the workarounds that I have in place and correctly handle the atexit functions in a consistent manner that should work on all platforms that implement Thread Local Storage. For platforms where TLS is unavailable, I do have a work-in-progress patch that uses pthread_set_specific as a fallback, so we can use DILL_TLS_FALLBACK flag to enable it.

sustrik commented 7 years ago

My naive feeling would be to make everything work using pthread_* functions incuding getspecific() first as those are more or less guaranteed to work. Then add optimisations for getspecific() as needed -- context creation/destruction are not performance sensitive, so it's not worth optimising them anyway.

That being said, let's merge this patch first and discuss possible simplifications afterwards. I see it passes the tests now. Do you consider it ready?

raedwulf commented 7 years ago

I was just thinking about the pthread functions and yes it would be make sense though the compiler won't be able to optimise accesses to the TLS. So we'd have to pick up the context from the API entry point and pass it down as an argument to internal functions.

The current code is ready now - I made dill_atexit always pass the relevant context to its destructors which simplifies all the destructor cod.

sustrik commented 7 years ago

Ok. Merged. Great work, btw! If it weren't for you I would probably just ignore multi-threaded scenarios expecting that supporting that would drag in too much ugliness. But in the end, this solution is surprisingly elegant.

sustrik commented 7 years ago

Btw, I'm working on merging all the partial contexts (handle,cr,pollset) into a single structure (single-context branch) . My feeling is that it will eventually simplify the thread-handling code. I'll send the patch your way when I'm done.

raedwulf commented 7 years ago

That sounds good! Yes it would simplify the handling code significantly!

sustrik commented 7 years ago

Ok, give it a look here: https://github.com/sustrik/libdill/tree/single-context

In theory, less indirection means that it should be faster, but I haven't checked. It may as well be that there are performance regressions.

raedwulf commented 7 years ago

It looks good and is faster on my system :)

sustrik commented 7 years ago

Great. Merged.

sustrik commented 7 years ago

Uh. Now I've noticed that the thread destructor isn't called for the main thread. Will investigate later on.

raedwulf commented 7 years ago

The thread destructor doesn't get called for the main thread. It only works with spawned threads - that's why I had dill_atexit check if it was running in the main thread with dill_ismain or not.

sustrik commented 7 years ago

Ok, I see. Fixed.