RFR: 8132510: Replace ThreadLocalStorage with compiler/language-based thread-local variables

[Thomas Stüfe]

Hi David,

On Thu, Nov 26, 2015 at 12:31 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> Thanks for reviewing.
>
> On 26/11/2015 1:20 AM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> I looked over your latest version at
>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v8
>>
>> This all looks nice and builds on AIX. I am currently running some jtreg
>> tests.
>>
>
> Thanks. Please let me know if any issues surface.


Sorry for the delay. AIX tests went through without any issues related to
your change. Our machines are terribly slow, so all I could do was
hotspot/runtime though.

But even if issues surface, we can easily backpaddle later to pthread tls.

Kind Regards, Thomas



> I will need to resubmit my own tests after latest changes.
>
> Some small remarks:
>>
>> ------------
>>
>> would it be possible to add a configure option (e.g.
>> --enable-compiler-based-tls)? On AIX, disabling compiler level tls also
>> makes the -qtls compiler option unnecessary, so it affects the makefile.
>>
>> Not really important, just would be quite comfortable.
>>
>
> It's possible to file a RFE for this. To be honest I didn't see this as
> some kind of user-defined choice that would need to be configured. The
> intent is that platforms which can't support compiler level TLS simply
> hardwire the flag in their platform specific makefile.
>
> ------------
>>
>>
>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v8/src/os/windows/vm/os_windows.cpp.udiff.html
>>
>> @@ -5175,11 +5159,11 @@
>> -  JavaThread* thread = (JavaThread*)(Thread::current());
>> +  Thread* thread = Thread::current();
>>     assert(thread->is_Java_thread(), "Must be JavaThread");
>>     JavaThread *jt = (JavaThread *)thread;
>>
>> may be simplified to use your new
>>
>> JavaThread* thread = JavaThread::current();
>>
>
> Indeed it can. Fixed. And changed jt to thread in the following code.
>
> ------------
>>
>>
>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v8/src/share/vm/runtime/thread.cpp.udiff.html
>>
>> in
>>
>> Thread::clear_thread_current()
>>
>> we could also add a
>>
>> assert(Thread::current() == ThreadLocalStorage::thread(), "TLS
>> mismatch!");
>>
>> to catch a case where library tls slot content changed during lifetime
>> of thread.
>>
>
> Fixed.
>
> ----
>>
>>   JavaThread* JavaThread::active() {
>> -  Thread* thread = ThreadLocalStorage::thread();
>> +  Thread* thread = Thread::current();
>>     assert(thread != NULL, "just checking");
>>
>> the assert is unnecessary.
>>
>
> Fixed.
>
>>
>> -----
>>
>>
>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v8/src/share/vm/runtime/thread.hpp.udiff.html
>>
>>   // Inline implementation of JavaThread::current
>>   inline JavaThread* JavaThread::current() {
>> -  Thread* thread = ThreadLocalStorage::thread();
>> +  Thread* thread = Thread::current();
>>     assert(thread != NULL && thread->is_Java_thread(), "just checking");
>>     return (JavaThread*)thread;
>>   }
>>
>> asserting thread != NULL is unnecessary.
>>
>
> Fixed
>
>> -----
>>
>>
>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v8/src/share/vm/utilities/globalDefinitions_gcc.hpp.udiff.html
>>
>> (and corresponding globalDefinitions_.. files)
>>
>> +#define THREAD_LOCAL_DECL __thread
>>
>> could be surrounded by #ifndef USE_LIBRARY_BASED_TLS_ONLY
>>
>
> Okay - changed.
>
> Updated webrev: http://cr.openjdk.java.net/~dholmes/8132510/webrev.v9/
>
> Thanks,
> David
> -----
>
> -----
>>
>> Kind Regards, Thomas
>>
>>
>>
>>
>>
>>
>>
>>
>> On Mon, Nov 23, 2015 at 8:03 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     After all the preliminary discussions here are final proposed changes:
>>
>>     bug: https://bugs.openjdk.java.net/browse/JDK-8132510
>>
>>     Open webrev: http://cr.openjdk.java.net/~dholmes/8132510/webrev.v6/
>>
>>     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 os_cpu-specific ThreadLocalStorage
>>     implementations, and the associated os::thread_local_storage* calls.
>>
>>     As __thread is not async-signal-safe (either in theory or practice)
>>     we need a fallback library-based solution as used today. For this we
>>     use a very simple ThreadLocalStorage class and an implementation
>>     thereof for POSIX (covers AIX, BSD, Linux, OSX and Solaris) using
>>     pthread_get/setspecific; and one for Windows using its TLS library.
>>     While these library routines are not guaranteed async-signal-safe,
>>     they seem to be in practice and are what we have been using all along.
>>
>>     We also allow for use of only the library-based TLS for platforms
>>     where compiler-based thread locals are not supported (as will be
>>     needed in the Mobile project). This is enabled at build time by
>>     defining USE_LIBRARY_BASED_TLS_ONLY.
>>
>>     Thanks to Andrew Haley for providing the Aarch64 code; and for
>>     Thomas Stuefe for testing PPC and AIX.
>>
>>     Testing:
>>       - JPRT (core platforms)
>>       - Jtreg tests (linux & solaris)
>>       - vm.runtime (core platforms)
>>
>>     Performance:
>>       - still TBD - this is proving to be extremely difficult. If anyone
>>     has any simple to run microbenchmarks to suggest I'd give them a try
>>     as a sanity check. But I lack access to hardware for running serious
>>     benchmarking.
>>
>>     Footprint:
>>     - varies by platform and the VM (server, client, minimal)
>>     - worst-case: ~1% increase for server VM and minimal VM
>>     - best-case:  0.4% decrease for client VM
>>
>>     Thanks,
>>     David
>>
>>
>>