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

Wed Oct 28 15:09:00 UTC 2015

Sounds good.

Thanks,
Vladimir

On 10/28/15 10:58 PM, Zoltán Majó wrote:
> 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 >>> >
>>>>>
>>>>>
>>>>
>>>
>>
>