RFR(S): 8179953: [ppc] TLABWasteIncrement not loaded correctly

[David Holmes]

Hi Goetz,

On 15/05/2017 8:31 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> yes, what you are saying is correct, and  I share your concerns about the
> design of JVMOptionsUtils.
> Actually I think a test is considered as passed if the VM issues the message
> that gc flags arecontradictory.  Thus, if you run with GCType=ParallelGC those
> flags that set G1GC explicitly are not properly tested. This is what first
> happened with my fix as we test with default settings which is G1.
> And here I didn't consider that I might change test behavior for the other
> flags.  But if this raises new errors, they should be addressed anyways.

I'm trying to test this change with each explicit GC flag but the thing 
is so slow that it keeps timing out on me. Will take about 5 or 6 hours 
to test at this rate.

David

> Best regards,
>   Goetz.
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Montag, 15. Mai 2017 06:25
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(S): 8179953: [ppc] TLABWasteIncrement not loaded correctly
>>
>> Hi Goetz,
>>
>> On 12/05/2017 4:45 PM, Lindenmaier, Goetz wrote:
>>> Hi David,
>>>
>>> the broken code is protected by
>>>    if (allow_shared_alloc) {
>>> which evaluates to true with ParallelGC but not with G1GC.
>>> It might be true with other GCs, too, but the design of
>>> JVMOptionsUtils:addNameDependency() is quite simple,
>>> and I just did it as for other flags.
>>> I could put in more logic into that case there, like
>>>    if (GCType == G1GC || GCTyep == ... ) {
>>>          option.addPrepend("-XX:+UseParallelGC");
>>>    }
>>> enumerating all GCs that don't support TLABWastIncrement.
>>> But I thought one test case for the flag is fine.  The way it is
>>> we missed the error, so I would like to fix the test.
>>
>> So if I understand correctly you just want to test use of
>> TLABWasteIncrement, and you know ParallelGC uses it, so you added
>> -XX:+UseParallelGC to the test options. Okay I get that part.
> Yes.
>
>> However the more I look at JVMOptionsUtils.java the less I understand
>> how it expects things to work. I don't find addNameDependency simple,
>> but confused! It looks for things like G1* and so adds UseG1GC, but
>> completely ignores that GCType may already be set - but that works
>> because AFAICS when it finds a G1* option in the existing set of
>> options, it has to be that G1GC is already the current GC type -
>> otherwise the test would fail - no?
>>
>> Looking at the history of this code it never set the GC so would use the
>> default, unless addNameDependency explicitly set one. That worked fine.
>> Then 8144578 took issue with the fact the test only ran with the default
>> GC and so introduced GCType to run the test using the same GC as was
>> used to launch the main test. But AFAICS that completely overlooked the
>> fact that addNameDependency might set an explicit GC! Mea culpa as I was
>> one of the reviewers of that change and it was obviously incomplete! :(
>> It also seem there are mails missing from the archives as you find now
>> mail from me here:
>>
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-January/021390.html
>>
>> BTW in that review it refers to TLABWasteIncrement as a "non-dedicated
>> gc flag". That suggests to me that testing of this flag was expected to
>> be GC agnostic. ??
>>
>> Anyway the question is, what is the right/best way to reconcile the use
>> of GCType with explicit setting of GC selection flags? Your fix is one
>> way. Another option would be to clear GCType when requiring a specifc
>> GC. Not sure either way is significantly better - pros and cons to both.
>>
>> My concern is whether this change may have an unexpected side-effects
>> given that I don't see how it can have been working correctly in the
>> first place.
>>
>> Unfortunately the primary author of that code is no longer around.
>>
>> David
>>
>>
>>> Yes, in JVMOption I must exclude setting G1GC else I get
>>> an error claiming that two different GCs are set.
>>>
>>> Best regards,
>>>   Goetz.
>>>
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Wednesday, May 10, 2017 5:19 AM
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(S): 8179953: [ppc] TLABWasteIncrement not loaded
>>>> correctly
>>>>
>>>> Hi Goetz,
>>>>
>>>> On 10/05/2017 12:56 AM, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this change.  As I fix the test, I please need a sponsor.
>>>>> http://cr.openjdk.java.net/~goetz/wr17/8179953-ppc_flag/webrev.01/
>>>>
>>>> I'm unclear on the test changes. It isn't obvious to me that
>>>> TLABWasteIncrement is a ParallelGC only flag. By adding that as an
>>>> additional flag you then had to exclude adding the GCType (if any GC
>>>> selector is given) to avoid conflicting options - is that right?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> On ppc, the mentioned flag is loaded as a 16 bit immediate, while it can
>>>> have
>>>>> bigger values.
>>>>> I also adapt TestOptionWithRanges, which did not show the bug because
>>>>> the flag has no effect with G1.
>>>>>
>>>>> Best regards,
>>>>>   Goetz.
>>>>>
>>>>>