[9] RFR(S): 8138651: -XX:DisableIntrinsic matches intrinsics overly eagerly
Zoltán Majó
zoltan.majo at oracle.com
Wed Oct 28 14:46:58 UTC 2015
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 >>> >
>>>
>>>
>>
>
More information about the hotspot-compiler-dev
mailing list