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

[Lindenmaier, Goetz]

Hi David, 

I made a new webrev which contains a proper patch with all the
change information:
http://cr.openjdk.java.net/~goetz/wr17/8179953-ppc_flag/webrev.02/

Sorry I didn't come back to this more early. Thanks for sponsoring!

Best regards,
  Goetz.


> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Mittwoch, 17. Mai 2017 11:50
> 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
> 
> 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.
> >>>>>>
> >>>>>>