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

[David Holmes]

Sorry about that:

http://cr.openjdk.java.net/~dholmes/8132510/webrev.v7/

David

On 24/11/2015 11:17 AM, David Holmes wrote:
> Updated webrev:
>
> cr.openjdk.java.net/~dholmes/8132510/webrev.v7/
>
> Only change is in macroAssembler_x86.cpp
>
> Thanks,
> David
>
> On 24/11/2015 7:42 AM, David Holmes wrote:
>> On 23/11/2015 11:46 PM, Bertrand Delsart wrote:
>>> On 23/11/2015 13:27, David Holmes wrote:
>>>> Hi Bertrand,
>>>>
>>>> On 23/11/2015 8:14 PM, Bertrand Delsart wrote:
>>>>> Hi David,
>>>>>
>>>>> Overall looks good.
>>>>> Thanks for the #ifndef USE_LIBRARY_BASED_TLS_ONLY :-)
>>>>>
>>>>> One doubt, in case this has not been discussed before.
>>>>>
>>>>> I'm still catching up on x86_64 (after years of ARM assembly :-))
>>>>> but it
>>>>> seems that there are some stack alignment constraints on runtime
>>>>> calls,
>>>>> at least for some x86_64 ABIs.
>>>>>
>>>>> Some of the x86 MacroAssembler::get_thread implementations had code to
>>>>> align the stack before calling pthread_getspecific. See for instance
>>>>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v6/src/os_cpu/linux_x86/vm/assembler_linux_x86.cpp.udiff.html
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> Sorry I'm not that familiar with the MacroAssembler - is it this odd
>>>> fragment:
>>>>
>>>> -   push(r10);
>>>> -   // XXX
>>>> -   mov(r10, rsp);
>>>> -   andq(rsp, -16);
>>>> -   push(r10);
>>>>
>>>> I'm not at all clear what that is doing - and if it somehow changes the
>>>> alignment wouldn't something need to be fixed up when popping the
>>>> previous values ??
>>>>
>>>> To be honest I'm not even sure what an "unaligned stack" means.
>>>
>>> On some platforms, SP may have to be aligned on a 16 bytes boundary when
>>> calling another method.
>>
>> Okay - that seems to be an x64 ABI requirement based on other code in
>> MacroAssembler. The reason it is missing in the Solaris code is because
>> I already removed it in the earlier changes that were done on Solaris:
>>
>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/d5b328043c10
>>
>> and nobody picked it up. :(
>>
>> That said, the absence of this code has not caused any problems so
>> presumably we are normally aligned.
>>
>>> After having pushed what needed to be saved, the code above rounds SP
>>> down and saves the old value. It will then also push r11, which results
>>> in the expected alignment.
>>>
>>> The conterpart, after the VM call, is the:
>>>   pop(r11);
>>>   pop(rsp);
>>
>> I had assumed this extra manipulation was related to pushing the arg for
>> the call.
>>
>>>>> This alignment is no longer performed in the new (shared)
>>>>> implementation
>>>>> http://cr.openjdk.java.net/~dholmes/8132510/webrev.v6/src/cpu/x86/vm/macroAssembler_x86.cpp.udiff.html
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Now, Solaris was not performing the alignment and Windows has a
>>>>> separate
>>>>> path for x86_64. Did we really need the alignment for linux x86_64 and
>>>>> bsd_x86_64 ? Might it be needed for other ports ?
>>>>>
>>>>> IMHO, it might be safer to align the stack by default, knowing it
>>>>> should
>>>>> not be expensive since we call get_thread rarely for x86_64 (result is
>>>>> cached in r15). I'll let you see whether it is worth adding an
>>>>> ifdef so
>>>>> as to explicitly deactivate the alignment on some platforms
>>>>> (solaris_x86_64 ?)
>>>>
>>>> I don't have enough knowledge to even begin to comment on this. I will
>>>> have to rely on our x86_64 macroassembler experts to explain the old
>>>> code and what the requirements are.
>>>
>>> OK. This means that the alignment was not removed purposefully.
>>>
>>> In that case, you must either keep the per platform code x86_64 (since
>>> it was not identical for all platforms) or use the safest version, with
>>> the additional
>>>
>>>     // XXX
>>>     mov(r10, rsp);
>>>     andq(rsp, -16);
>>>     push(r10);
>>>
>>> before the push(r11) and with the
>>>
>>>     pop(rsp);
>>>
>>> after the pop(r11). It should work on all x86_64 platforms.
>>
>> Right, I will add this back to the code - and replace the meaningless //
>> XXX with an actual comment!
>>
>> Many thanks,
>> David
>> -----
>>
>>> FYI, there is another way to do the alignment, based on the fact that
>>> we are at least aligned on 8 bytes (see
>>> MacroAssembler::call_VM_leaf_base). I assume that this second version is
>>> more efficient (particularly thanks to specilative branches) but it
>>> might be safer/simpler to continue using andq in get_thread.
>>>
>>> Bertrand.
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Regards,
>>>>>
>>>>> Bertrand.
>>>>>
>>>>>
>>>>>
>>>>> On 23/11/2015 08:03, David Holmes 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
>>>>>
>>>>>
>>>
>>>