[9] RFR(S): 8138651: -XX:DisableIntrinsic matches intrinsics overly eagerly

[Zoltán Majó]

Hi Nils,


thank you for the feedback!

On 10/23/2015 01:49 PM, Nils Eliasson wrote:
> Hi Zoltan,
>
> Some comments:
>
> IntrinsicDisabledTest.java:
>
> >> return >> System.getProperty("java.vm.name").toLowerCase().contains("server");
> >>
>
> think the best practice is to use Platform.isServer() ("import 
> jdk.test.lib.Platform;").

I did not know about that method. Thanks, I've updated the code.

>
> compilerDirectives.cpp:
>
> I think the canonilization of the list belongs at the construction 
> site, and not do at the (hot) use site.

The call sites of DirectiveSet::is_intrinsic_disabled() are not that 
hot, as they are called either when a method is compiled or through the 
WhiteBox API. In the first case, the time spent on going through a small 
character array containing disabled intrinsics is not be high (relative 
to the time spent on compilation). The WhiteBox API is -- I think -- not 
available in product builds.

But from a design perspective it might be a better design to 
canonicalize the string on construction.

> Preferably we would agree on using the ',' separator in all use case 
> (it only has internal uses). The compilecommand parser should be 
> straightforward to fix. The VM flag may be parsed by a common parser 
> that we can't change - then the vmflag value should be canonicalized 
> during CompilerBroker_init or similar.

There are other flags of type ccstrlist. Changing the way a ccstrlist 
flags are parsed might affect these as well, so I would not want to 
change the way the VM parses flags.

>
> If there is some reason to why that doesn't work then I would suggest 
> moving the canonicalization to DirectiveSet constructor and 
> DirectiveSet::clone so it only happens once per DirectiveSet.

That is a good idea. The new webrev performs canonicalization
- in the DirectiveSet constructor, when the value of the global 
DisableIntrinsic flag is read;
- in the DirectiveSet::compilecommand_compatibility_init() method, when 
the value of the per-method DisableIntrinsic flag is read.

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8138651/webrev.02/

I've tested the updated webrev with:
- JPRT (testset hotspot), all tests pass;
- locally executing all hotspot tests, all tests pass that pass with the 
unmodified version of the VM.

Thank you and best regards,


Zoltan

>
> Best regards,
> Nils
>
>
>
>
>
> On 2015-10-23 09:51, Zoltán Majó wrote:
> > Hi Vladimir, > > > thank you for the feedback! > > On 10/07/2015 04:37 AM, 
> Vladimir Kozlov wrote: >> To be precise DisableIntrinsic is ccstrlist 
> option, not ccstr. Yes, >> the actual type is the same. >> >> An other 
> concern is separators since format could be different if >> option 
> specified in file. Look how we do search in DeoptimizeOnlyAt >> 
> string. > > I've checked and DisableIntrinsic supports accumulation of 
> argument > values: If -XX:DisableIntrinsic is specified multiple times 
> on the > command line, all argument values are concatenated into one 
> argument. > In that case, '\n' is used as separator. I updated the 
> webrev to > support "\n" as a separator. > > If DisableIntrinsic is 
> used on the per-method level (i.e., with > 
> -XX:CompileCommand=option,...), HotSpot expects the type of > 
> DisableIntrinsic to be 'ccstr' and not 'ccstrlist'. That does not > 
> allow specifying a list of intrinsics to be disabled (e.g., > 
> _getInt,_getInVolatile,_hashCode) and is inconsistent with the > 
> declaration of DisableIntrinsic in globals.hpp. > > To address this 
> problem, the webrev changes the type of the > per-method level 
> DisableIntrinsic flag to 'ccstrlist'. For per-method > ccstrlists, the 
> separator is a whitespace (internally). I've updated > the webrev to 
> support whitespace as a separator as well. > > I noticed an other 
> problem while working on this fix: If > DisableIntrinsic is specified 
> multiple times for the same method, > argument values do not 
> accumulate. For example, with > > 
> -XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_getInt,_getIntVolatile 
> > >
> -XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_hashCode
> >> only '_hashCode' will be disabled for 'putChar'. That is also > 
> inconsistent with the way DisableIntrinsic works when used globally > 
> (with -XX:DisableIntrinsic). > > This inconsistency should be 
> addressed, but as the fix requires > significant changes to 
> CompilerOracle, I would like to take care of > that separately. I've 
> filed an RFE for that: > > 
> https://bugs.openjdk.java.net/browse/JDK-8140322 > > I hope that is 
> fine. > > Here is the updated webrev: > 
> http://cr.openjdk.java.net/~zmajo/8138651/webrev.01/ > > Testing: - 
> JPRT (testset hotspot, including the newly added > 
> IntrinsicDisabledTest.java test). > > Thank you and best regards, > > 
> > Zoltan > >> >> Thanks, Vladimir >> >> On 10/6/15 8:00 PM, Zoltán 
> Majó wrote: >>> Hi, >>> >>> >>> please review the patch for 
> JDK-8138651. >>> >>> https://bugs.openjdk.java.net/browse/JDK-8138651 
> >>> >>> Problem: The DisableIntrinsic flag does not disable intrinsics 
> >>> accurately. For example, -XX:DisableIntrinsic=_copyOfRange >>> 
> disables both the intrinsic with the ID _copyOfRange and the >>> 
> intrinsic with the _copyOf. >>> >>> Solution: Change the processing of 
> the DisableIntrinsic flag >>> (both globally and on a per-method 
> level). >>> >>> Webrev: 
> http://cr.openjdk.java.net/~zmajo/8138651/webrev.00/ >>> >>> Testing: 
> - JPRT (testset hotspot); - executed the the newly added >>> test 
> compiler/intrinsics/IntrinsicDisabledTest.java with/without >>> the 
> fix on all platforms, the test behaves as expected. >>> >>> Thank you 
> and best regards, >>> >>> >>> Zoltan >>> >
>
>