RFR: 8230877: Rename THREAD_LOCAL_DECL to thread_local

[David Holmes]

Hi Per,

On 12/09/2019 6:48 pm, Per Liden wrote:
> 
> On 9/12/19 2:04 AM, Kim Barrett wrote:
>>> On Sep 11, 2019, at 7:46 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> But as we are not actually enabling this as we don't turn on C++11, 
>>> surely this still "looks good" enough? But we will have to look 
>>> closer at things before turning it on? We may need to clarify our own 
>>> rules for using thread-locals if we find there is a performance hit 
>>> and we want to use the magic flag.
>>
>> Part of the rationale for renaming to thread_local was that the
>> transition to C++11 was trivial.  But apparently not so.
>>
>> I really don't want to be tripping over this hidden performance hit
>> when trying to throw the language standard switch.  (Assuming it's
>> measurable, but we hit Thread::current() quite a bit, and ZGC has a
>> bunch of thread-local variables pretty baked in and used fairly
>> heavily I think.)  I do not want this little syntactic nicety to be an
>> argument for not throwing that switch.
>>
>> And in the long run, I'd prefer a different name if we're going to
>> effectively have non-standard semantics (because of the usage
>> restrictions required by that compiler flag).
>>
> 
> Ok, thanks for spotting the difference in gcc.
> 
> Given that they are said to be equivalent on windows and solaris, it 
> sounds like we're already taking the dynamic init hit on those 
> platforms. That's not necessarily a good reason to take that penalty on 
> linux too, just an observation.

Seems more like an assumption :) Who said there is any "dynamic init 
hit" on those platforms?

> I looked closer at what the overhead is with gcc. For fundamental types 
> and pointers (like our Thread::_thr_current), there's an extra 
> test-and-branch sequence before the load.
> 
>      cmpq    $0, xxxx at GOTPCREL(%rip)
>      je    done
>      call    slow_path
> done:
> 
> I've confirmed that -fno-extern-tls-init removes that test-and-branch.
> 
> I'm not super keep on spending too much time on investigating if this 
> overhead is negligible or not. I also agree that messing with the 
> thread_local semantics using -fno-extern-tls-init, or calling something 
> thread_local that has non-standard semantics seems undesirable.
> 
> So, how about I dial this back a bit, and just rename THREAD_LOCAL_DECL 
> to THREAD_LOCAL, just to make it a bit less clunky? That doesn't give us 
> all the benefits I originally had in mind, but given the constraints we 
> have here I don't see any better alternatives.
> 
> Updated webrev: http://cr.openjdk.java.net/~pliden/8230877/webrev.2

The rename to drop _DECL is fine and trivial.

Removing the USE_LIBRARY_BASED_TLS_ONLY guard makes it harder to test 
with compiler-thread-local disabled. But not drastically so.

So I'm okay with this minor change.

Thanks,
David

> /Per