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

Wed Oct 28 14:10:08 UTC 2015

Hi Zoltan,

Thank you, it looks much better.

I dislike the extra copy forced by using strtok in 
is_intrinsics_disabled. I tried rewriting it using strstr + checking 
trailing token but it didn't get any cleaner so I am ok with it.

Make sure to run all the regression tests for compiler control in 
test/compiler/compilercontrol

Best regards,
Nils

On 2015-10-26 15:16, Zoltán Majó wrote:
> 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 >>> >
>>
>>
>