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

[Lindenmaier, Goetz]

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.

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