RFR (XS) 8202360: [TESTBUG] runtime/LoadClass/TestResize.java needs to print output when it fails

[Gerard Ziemski]

> On May 24, 2018, at 8:03 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> This has really become far too much work for what the intent of the bug report was. Sorry.
> 
> On 24/05/2018 5:29 AM, Gerard Ziemski wrote:
>> hi David,
>>> On May 21, 2018, at 5:34 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> <trimming>
>>> 
>>> On 22/05/2018 6:57 AM, Gerard Ziemski wrote:
>>>>> On May 20, 2018, at 8:38 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>> Okay - that helps with the immediate problem I had. I also now realize that the failure in this test is showing is that the SD did not in fact re-size as expected!
>>>> Can you elaborate? Is there a reproducible case I can try to run?
>>> 
>>> The test's purpose is to check that the SD resizes, so if it finds a load factor too high (which indicates it didn't resize) then the SD resizing logic has either not worked as expected, or the test is not doing what it thinks!
>> The resizing is called during a safe point, which we try to trigger by issuing a System.gc() call (in TriggerResize.java). If no safe point takes place, then no resize had chance to occur, and we have test failure, which is probably what you experienced.
>> I changed the test to look for safe points in the output and check the load factor only when resizing had the chance to occur.
> 
> I'm losing confidence in what this test actually tests. For GC's that ignore explicit GC requests via System.gc() this test is never going to test anything e.g. using default G1 GC.

The assumption here is that the safe point will be triggered by VM during the test on its own, and it always did in my local testing (but apparently not for you at least once). We don’t rely on System.gc() - it’s just the last resort, which btw seems to be doing the right thing on my Mac.


> 
>>>> I have re-done the main logic a bit to simplify it, and not have to do either String and “+” nor StringBuffer, by taking advantage of the fact that the info we need to parse is on the 2nd output line.
>>> 
>>> I was under the impression that we could see multiple lines of the form:
>>> 
>>> Java dictionary (table_size=40423, classes=50002)
>>> 
>>> as the table resized. If that is not the case then none of the output seems relevant to me except for this one line. ??
>> That print out comes from PrintSystemDictionaryAtExit, which happens only once at the exit, but there is more then one system dictionary, and all of them are checked.
> 
> But now I understand all that, the dumping of all the output really doesn't help in establishing why the test failed. My original request was for more information to see why the load factor was too high. I thought, incorrectly, that we would see a series of changing size+classes reports until we suddenly hit a bad load factor. But that isn't the case. All that was missing in the end were the actual table_size and classes values that caused the error. Nothing more, nothing less.
> 
>> https://bugs.openjdk.java.net/browse/JDK-8202360
>> http://cr.openjdk.java.net/~gziemski/8202360_rev4
> 
> String already has a contains method you don't need to write your own helper.

String.contains(CharSequence s) API takes CharSequence, not String.


> 
> I don't see why you stopped using BufferedReader to read each line, and replaced with Scanner.

Personally, I find it simpler to convert the entire output into String, which then can be tokenized into lines, which then can be looked at one at a time in a simple “for loop”. This also avoids the issue of remembering to consume all the output to unblock the process.


> With the scanner the split into lines:
> 
> String[] lines = output.split("\n");
> 
> should be:
> 
> String[] lines = output.split("\\\R”);
> 
> 
> to ensure no issues with platform-specific line terminators.

Done.


> 
> 83       if (line.startsWith("[")) {
> 
> You presumably only need to know one resize has occurred so this can be guarded with !resize.

Done.


> 
> 99           // We've hit an error, so print all of the output.
> 100           System.out.println(output);
> 
> No need to do this. And I'm not even sure what output contains any more.

If we ever run into such issue again, it will be helpful to know which dictionary triggered it, and what it contained. Any additional info, such as did the safe point trigger and did it attempt to resize will be helpful as well.


> 
> 107         }
> 
> Once you get here the test is over, one way or another - you saw it resized and you found the Java Dictionary line. So at this point you should terminate the loop and then (if needed) finish processing the output so the target can terminate.

There is more than one system dictionary, so we need to keep processing the output to check them all.


> 
> Finally if no resize occurred you should print a message that the test trivially passed without any resizing occurring eg:
> 
> if (!resized) {
>  System.out.println("Test considered PASSED - but no resizing occurred.");
> }

Done


> 
> Again I'm sorry this has turned into such an issue.

I don’t want anyone else to get tripped off by this test, so getting it right is worth it imho. Also, once finished, it will be a nice test template for anyone else who needs to process the output.

Thanks for your patience!

https://bugs.openjdk.java.net/browse/JDK-8202360
http://cr.openjdk.java.net/~gziemski/8202360_rev5


cheers