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

[David Holmes]

Okay testing passed.

If you finalize the changeset and send me a link I will sponsor it for you.

Thanks,
David

On 17/05/2017 5:01 PM, David Holmes wrote:
> 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.
>>>>>>
>>>>>>