RFR: 8230877: Rename THREAD_LOCAL_DECL to thread_local
david.holmes at oracle.com
Thu Sep 12 10:53:36 UTC 2019
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>
>>> 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
> 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.
More information about the hotspot-dev