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

[David Holmes]

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. 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
>
>