(L) Prelim RFR: 8132510: Replace ThreadLocalStorage with compiler/language-based thread-local variables

[Thomas Stüfe]

Hi David,

On Mon, Nov 2, 2015 at 9:01 PM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 2/11/2015 9:20 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> some small changes are needed to make this build and run on AIX. I
>> attached a patch file with the needed additions.
>>
>
> Thanks!
>
>
I also checked Linux ppc64, seems to work fine.



> I did not run any extensive tests on AIX, so I cannot say for sure if
>> this is stable. We (SAP) also may face some problems later when we port
>> this to HP-UX, because there, shared libraries using __thread cannot be
>> loaded dynamically.
>>
>
> Ouch!
>
> So, I admit to some small worries, beside the issue with memory leaks on
>> older glibc versions. For me, this feels like something which needs
>> tight compiler/thread library support from the OS, so it makes us
>> vulnerable to running on older systems (older glibc) or building with
>> outdated compilers. Therefore it would be nice to have a simple way to
>> re-add the pthread-based TLS implementation if needed.
>>
>
> I can't see how to do that without keeping all the existing layers of code
> - even though they would be no-ops on all the platforms that support the
> compiler-based TLS. Basically just extend what I did for Solaris to the
> other platforms.


I took a closer look and I now I worry less. I am confident that in case
our old platforms experience problemswith __thread, we can reintroduce TLS
without too many changes.

Just as a test, I changed the AIX implementation from using __thread back
to pthread tls just by changing implementations for Thread::current(),
Thread::initialize_thread_current() and Thead::clear_thread_current() in
thread.cpp. Works fine as expected. Of course this was just a hack, but if
we need to go back to pthread tls for AIX or any platform, I think it can
be done in a simpler way than before and still be clean.

Not terribly important, but I would prefer if
Thread::initialize_thread_current() and Thead::clear_thread_current() were
not exposed from Thread at all or at least as private as possible.
Thread::initialize_thread_current() is called from the OS layer, but
Thead::clear_thread_current() is only called from within thread.cpp itself
and could be kept at file scope.


>
>
> Apart from that, I like the patch and think the simplification is good
>> and worth the effort.
>>
>
> Even if you can't easily add back the pthread-based TLS if needed?
>

I think we can, if needed.



> It is unfortunate that hotspot may still be shackled to the past that way
> - we killed off hotspot-express (in part) to remove those shackles and
> allow us to modernize the codebase.
>
> Thanks,
> David
>
>
One question about your changes:

Before, Thread::current() would assert instead of returning NULL if called
before Thread::initialize_thread_current() or after
Thead::clear_thread_current() . Now, we just return NULL. Is this intended?

Regards, Thomas



> Kind Regards, Thomas
>>
>>
>>
>>
>>
>>
>> On Mon, Nov 2, 2015 at 7:40 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     bug: https://bugs.openjdk.java.net/browse/JDK-8132510
>>
>>     Open webrev: http://cr.openjdk.java.net/~dholmes/8132510/webrev.v2/
>>
>>     A simple (in principle) but wide-ranging change which should appeal
>>     to our Code Deletion Engineer's. We implement Thread::current()
>>     using a compiler/language-based thread-local variable eg:
>>
>>
>>       static __thread Thread *_thr_current;
>>
>>       inline Thread* Thread::current() {
>>         return _thr_current;
>>       }
>>
>>     with an appropriate setter of course. By doing this we can
>>     completely remove the platform-specific ThreadLocalStorage
>>     implementations, and the associated os::thread_local_storage* calls,
>>     plus all the uses of ThreadLocalStorage::thread() and
>>     ThreadLocalStorage::get_thread_slow(). This extends the previous
>>     work done on Solaris to implement ThreadLocalStorage::thread() using
>>     compiler-based thread-locals.
>>
>>     We can also consolidate nearly all the os_cpu versions of
>>     MacroAssembler::get_thread on x86 into one cpu specific one ( a
>>     special variant is still needed for 32-bit Windows).
>>
>>     As a result of this change we have further potential cleanups:
>>     - all the src/os/<os>/vm/thread_<os>.inline.hpp files are now
>>     completely empty and could also be removed
>>     - the MINIMIZE_RAM_USAGE define (which avoids use of the linux
>>     sp-map "cache" on 32-bit) now has no affect and so could be
>>     completely removed from the build system
>>
>>     I plan to do the MINIMIZE_RAM_USAGE removal as a follow up CR, but
>>     could add the removal of the "inline" files to this CR if people
>>     think it worth removing them.
>>
>>     I have one missing piece on Aarch64 - I need to change
>>     MacroAssembler::get_thread to simply call Thread::current() as on
>>     other platforms, but I don't know how to write that. I would
>>     appreciate it if someone could give me the right code for that.
>>
>>     I would also appreciate comments/testing by the AIX and PPC64 folk
>>     as well.
>>
>>     A concern about memory-leaks had previously been raised, but
>>     experiments using simple C code on linux 86 and Solaris showed no
>>     issues. Also note that Aarch64 already uses this kind of thread-local.
>>
>>     Thanks,
>>     David
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/porters-dev/attachments/20151103/8559b015/attachment.html>