RFR(XL): 8185640: Thread-local handshakes

[David Holmes]

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.

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

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