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

[David Holmes]

Hi Goetz,

We need a second reviewer. :(

David

On 23/05/2017 4:23 PM, Lindenmaier, Goetz wrote:
> 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.
>>>>>>>>
>>>>>>>>