RFR(XL): 8185640: Thread-local handshakes

[David Holmes]

On 1/11/2017 7:37 PM, Robbin Ehn wrote:
> 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.

Okay. I find it a bit confusing to have the flag processing logic split 
in two like this. But I'll leave it at that.

Thanks,
David

> /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
>>>>>>
>>>>>>
>>>>>>