RFR(XL): 8185640: Thread-local handshakes

[David Holmes]

Hi Robbin,

On 31/10/2017 12:34 AM, Robbin Ehn wrote:
> Thanks!
> 
> There have been a bit hesitation and confusion about the option (at 
> least internally).
> The option is opt-out but in globals.hpp it starts out as false.
> 
> Now instead we explicit set it true in globals.hpp but we turn it off if 
> we notice that:
> - We are on an unsupported platform
> - User have specified UseAOT
> - User have specified EnableJVMCI

That logic from #4617 onwards is absolutely doing my head in!

4617   bool aot_enabled = UseAOT && ((AOTLibrary != NULL) || 
!FLAG_IS_DEFAULT(UseAOT));

why do we care if the flag is default or not? If they set an AOTLibrary 
they expect AOT to be enabled. If they know UseAOT is true by default 
then they won't set it explicitly and so the flag will be default. If 
they set UseAOT directly but don't set a library then they won't get AOT 
- and UseAOT should be turned off somewhere else.

4623     if (FLAG_IS_DEFAULT(UseAOT)) {

Why do we care if it is default or not? If we got here AOT is not 
enabled. We can just do:

if (UseAOT) FLAG_SET_DEFAULT(UseAOT, false)

or even skip the query and just set it false.

4627     if (FLAG_IS_DEFAULT(ThreadLocalHandshakes) && 
ThreadLocalHandshakes) {

Okay I get why you check for default here :)

4631         FLAG_SET_ERGO(bool, ThreadLocalHandshakes, false);

I wouldn't really say this is an "ergo" choice - if we can't have it on 
then we set it off - just as previously done with UseAOT.

4632       } else if (!FLAG_IS_DEFAULT(UseAOT) && UseAOT) {

Again why do we care about default? You seem to be saying that "java 
-XX:+UseAOT -XX:AOTLibrary=..." is a stronger request for AOT than just 
"java -XX:AOTLibrary=...". But I'd always use the latter if I know 
UseAOT defaults to true anyway.

4635         FLAG_SET_ERGO(bool, ThreadLocalHandshakes, false);
4639         FLAG_SET_ERGO(bool, ThreadLocalHandshakes, false);

Same ergo comment.

I'm also thinking, if this is platform dependent then shouldn't 
ThreadLocalHandshakes be a product_pd flag, with pd specific default 
setting - and turning it on when on an unsupported platform should be a 
error ?

Thanks,
David
-----

> Here is webrev for changes needed:
> http://cr.openjdk.java.net/~rehn/8185640/v8/Option-Cleanup-12/webrev/
> And here is CSR:
> https://bugs.openjdk.java.net/browse/JDK-8189942
> 
> Manual testing + basic testing done.
> 
> And since I'm really hoping that this can be the last incremental, here 
> is my whole patch queue flatten out:
> http://cr.openjdk.java.net/~rehn/8185640/v8/Full/webrev/
> 
> Thanks, Robbin
> 
> On 10/27/2017 04:47 PM, Doerr, Martin wrote:
>> Hi Robbin,
>>
>> excellent. I think this matches what Coleen had proposed, now.
>> Thanks for doing all the work with so many incremental patches and for 
>> responding on so many discussions. Seems to be a tough piece of work.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Robbin Ehn [mailto:robbin.ehn at oracle.com]
>> Sent: Freitag, 27. Oktober 2017 15:15
>> To: Erik Österlund <erik.osterlund at oracle.com>; Andrew Haley 
>> <aph at redhat.com>; Doerr, Martin <martin.doerr at sap.com>; Karen Kinnear 
>> <karen.kinnear at oracle.com>; Coleen Phillimore 
>> (coleen.phillimore at oracle.com) <coleen.phillimore at oracle.com>
>> Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
>> Subject: Re: RFR(XL): 8185640: Thread-local handshakes
>>
>> Hi all,
>>
>> Poll in switches:
>> http://cr.openjdk.java.net/~rehn/8185640/v7/Interpreter-Poll-Switch-10/
>>
>> Poll in return:
>> http://cr.openjdk.java.net/~rehn/8185640/v7/Interpreter-Poll-Ret-11/
>>
>> Please take an extra look at poll in return.
>>
>> Sanity tested, big test run still running (99% complete - OK).
>>
>> Performance regression for the added polls increased to total of 
>> -0.68% vs
>> global poll. (was -0.44%)
>>
>> We are discussing the opt-out option, the newest suggestion is to make it
>> diagnostic. Opinions?
>>
>> For anyone applying these patches, the number 9 patch changes the 
>> option from
>> product. I have not sent that out.
>>
>> Thanks, Robbin
>>
>>
>>