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

[Leo Korinth]

On 31/10/2019 10:51, Thomas Schatzl wrote:
> Hi,
> 
> On 28.10.19 11:40, Leo Korinth wrote:
>> 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 do not think either change makes a difference.
> 
>>
>> 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.
> 
> Updated in place; also fixed Kim's comment about line length.
> 
> http://cr.openjdk.java.net/~tschatzl/8232951/webrev/
> 
>>
>> 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.
> 
> The question I have is whether I can push these changes under this CR 
> (and if it occurs again we at least have a log to look at) or use 
> another CR for it?

I am fine with you pushing under the current CR.

Thanks,
Leo
> 
> Thanks,
>    Thomas