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

Wed Oct 28 14:58:51 UTC 2015

Hi,

For the record: I plan to push the latest webrev (webrev.02) tomorrow 
(Oct 29), unless, of course, there are other suggestions on how to 
improve this patch.

Thank you and best regards,

Zoltan

On 10/28/2015 03:46 PM, Zoltán Majó wrote:
> Hi Nils,
>
>
> On 10/28/2015 03:10 PM, Nils Eliasson wrote:
>> Hi Zoltan,
>>
>> Thank you, it looks much better.
>
> thank you for the review!
>
>>
>> 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.
>
> Thanks.
>
>>
>> Make sure to run all the regression tests for compiler control in 
>> test/compiler/compilercontrol
>
> I ran them, all tests pass that pass with an unmodified VM.
>
>>
>> Best regards,
>
> Thank you and best regards,
>
>
> Zoltan
>
>> 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 >>> >
>>>>
>>>>
>>>
>>
>