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

[David Holmes]

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.

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.
>>>
>>>