RFR (XS): 8232951: TestG1ParallelPhases.java fails with phase NonYoungFreeCSet not found

[Leo Korinth]

Hi.

Just want to add some information, because I think it will fail again.

The buggy test case is written by me and the provoke mixed gc part is 
copied mostly either from TestOldGenCollectionUsage or TestLogging (as 
it is hard to share this code due to JTREG). However when I did "copy" 
the code I also did try to improve the code, this could be the reason 
for this failure. I did at least two "improvements" in that I removed 
magic constants when allocating the 20k arrays and instead calculated 
how many I would need; this made the algorithm allocate ~2M instead of 
~3M which could be a problem although to my understanding it should not 
be. Another change I made is that I will not provoke a gc by allocating 
until out-of-memory. The original code seems to try to provoke a gc by 
starting concurrent marks and young gc, but kind of fail-safes with the 
code after the comment // allocate more objects to provoke GC. Having 
this code I guess would fix the problem with the test case, but on the 
other hand, we would not know why the youngGC() after concurrent mark 
does not provoke a mixed gc (I guess it should, but correct me if this 
is false).

I have talked to Thomas off-list, and I think AlwaysTenure is not the 
solution to the problem we have. I think adding the debug options is 
great and should be done, and AlwaysTenure seems better than 
MaxTenuringThreshold=1 but we should expect the test case to continue to 
fail in the future.

If you go by adding AlwaysTenure instead of MaxTenuringThreshold=1, 
please also remove one getWhiteBox().youngGC() in allocateOldObjects so 
that we do not leave "magic" lines in the test case. Also update the 
comment to // Do *one* young collections...
and there is another "-XX:MaxTenuringThreshold=1" that needs to be 
updated. I need no webrev for these changes.

I am sorry that my "improvements" probably caused this failure, though 
just having heaps of code and not understanding why, is probably worse 
in the long run --- at least that is my thinking.

Thanks,
Leo




On 28/10/2019 09:35, Stefan Johansson wrote:
> Hi Thomas,
> 
>> 26 okt. 2019 kl. 03:51 skrev Kim Barrett <kim.barrett at oracle.com>:
>>
>>> On Oct 24, 2019, at 7:50 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>> […]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8232951
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8232951/webrev/
>>> Testing:
>>> 400 runs of the changed test without issues
>>>
>>> Thanks,
>>> Thomas
>>
>> I'd not previously noticed the AlwaysTenure and NeverTenure options.
>> So many options...
>>
>> Those options are documented as being ParallelGC only.  But it looks
>> like setting either of them forces a value for MaxTenuringThreshold,
>> so it seems okay to change the test to use AlwaysTenure.  The
>> documentation for the options should be updated though.  (That can be
>> a separate RFE.)
>>
>> Please put the new -Xlog option on a separate line.  I know we don't
>> have an official line length limit, but 152 chars seems excessive to
>> me, and forced me to scroll to see some of it.
>>
>> Other than that, looks good.  I don't need a new webrev.
>>
> 
> Sounds like a good fix and it looks good,
> Stefan
>