[PATCH] Simplify TSD without TLS

[Jason Evans]

On Apr 18, 2012, at 9:25 AM, Mike Hommey wrote:
> On Tue, Apr 17, 2012 at 12:07:15PM +0200, Mike Hommey wrote:
>> 
>> It doesn't use a static variable when malloc_tsd_malloc
>> fails, but unconditionally aborts instead. Using a static variable is
>> due to fail with multithreading anyways. The 'initialized' variable is
>> not necessary either, as pthread_key's cleanup function won't be called
>> unless a non-NULL value has been set with pthread_setspecific.
> 
> With a more complete tsd testcase than the one I sent yesterday, I can
> see how the initialized variable is necessary. However, there's a bug,
> because it's not set to false when creating the wrapper, and this makes
> the cleanup function called even when tsd_set was never called.

Nice catch.

> There's also a corner case, with *something_tsd_get() = value. It
> doesn't trigger a cleanup at thread shutdown, and I'm not sure if that's
> a desired effect.

This is intended behavior; something_tsd_set() must be called to trigger cleanup.  pthreads tsd uses NULL/non-NULL to make the decision, but that doesn't quite make sense with this tsd API.

> Arguably, maybe this should be forbidden. Adding
> const to tsd_get() ensures that, but as a matter of fact,
> thread_allocated tsd does this kind of assignment. But it also doesn't
> have a cleanup function, so it's not really a problem. We may want to
> avoid such things from happening in the future, though.

Patch 7/11 (Disallow *_tsd_get() = value assignments) disallows intended functionality.  The thread_allocated code is on the fast path, and I don't want to make it any slower than it already is.

> Please note that patch 6/11 (Add a test for the TSD system) does *not*
> build without --enable-debug because it's such impossible build
> independent parts of jemalloc with optimized builds. You may however
> want to run the test on the freebsd implementation of the TSD system,
> because that's the only one I couldn't test. With patches 8 and 9, the
> --disable-tls implementation passes the test.

Patch 6/11 did indeed expose a bug in the FreeBSD implementation -- the cleanup function wasn't being called at all!  I've been thinking about how to include such tests, and two closely related ideas come to mind.  One is to add a --enable-unit-tests option that causes unit test entry points to be built into libjemalloc.  Then test programs can call those entry points.  I'm not super excited about this though, because it requires special build configuration to run all the tests.  The other option is to build a libjemalloc_test that has these entry points built in, and link all tests against that instead of libjemalloc.  What do you think?  Any other ideas?

> This patch queue closes on Mingw support. Next will be MSVC.


Patch 11/11 (Add support for Mingw) doesn't apply after the various changes I made while applying the rest of your patches (and leaving out 6/11 and 7/11).  Can you please rebase and resend?

Thanks,
Jason