RFR(XL): 8185640: Thread-local handshakes

[Robbin Ehn]

Hi David,

On 11/01/2017 02:23 AM, David Holmes wrote:
> Hi Robbin,
> 
> On 1/11/2017 12:37 AM, Robbin Ehn wrote:
>> Thank you David for having a look.
>>
>> I updated after your review, I think I got it all, please see:
>> http://cr.openjdk.java.net/~rehn/8185640/v9/DavidH-Option-Cleanup-13/webrev/ 
> 
> That looks much clearer - thanks.

Great!

> 
>> I'm also updating CSR with product_pd.
>>
>> Short thing:
>>
>> On 10/31/2017 11:27 AM, David Holmes wrote:
>>>
>>> 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 ?
>>
>> Yes, the error checking already exists in:
>>
>>   135 Flag::Error ThreadLocalHandshakesConstraintFunc(bool value, bool verbose) {
> 
> Ah I see. I'm not clear how this all hangs together. Is the constraint function 
> only called if the flag is set on the command-line? Is it called before or after 
> the other checking in arguments.cpp ?

It's always called after argument parsing.

/Robbin

> 
> Thanks,
> David
> 
>>   136   if (value) {
>>   137     if (!SafepointMechanism::supports_thread_local_poll()) {
>>   138       CommandLineError::print(verbose, "ThreadLocalHandshakes not yet 
>> supported on this platform\n");
>>   139       return Flag::VIOLATES_CONSTRAINT;
>>   140     }
>>   141     if (UseAOT JVMCI_ONLY(|| EnableJVMCI || UseJVMCICompiler)) {
>>   142       CommandLineError::print(verbose, "ThreadLocalHandshakes not yet 
>> supported in combination with AOT or JVMCI\n");
>>   143       return Flag::VIOLATES_CONSTRAINT;
>>   144     }
>>   145   }
>>   146   return Flag::SUCCESS;
>>   147 }
>>
>> Sanity tested with handshake benchmark on all supported + 1 unsupported platform.
>>
>> Thanks, Robbin
>>
>>>
>>> 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
>>>>>
>>>>>
>>>>>