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

[Gerard Ziemski]

hi David,

> On May 20, 2018, at 8:38 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Gerard,
> 
> On 19/05/2018 4:54 AM, Gerard Ziemski wrote:
>> hi David,
>>> On May 18, 2018, at 12:59 AM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> Hi Gerard,
>>> 
>>> On 18/05/2018 2:21 AM, Gerard Ziemski wrote:
>>>> hi David,
>>>> Thank you for the review!
>>>>> On May 16, 2018, at 4:54 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>> 
>>>>> Hi Gerard,
>>>>> 
>>>>> On 17/05/2018 3:47 AM, Gerard Ziemski wrote:
>>>>>> Hi all,
>>>>>> Please review this small enhancement where we print out additional information (i.e. output from PrintSystemDictionaryAtExit), when we detect the failure.
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8202360
>>>>>> http://cr.openjdk.java.net/~gziemski/8202360_rev1/
>>>>> 
>>>>> I don't understand what your fix is doing - sorry.
>>>> The fix takes all the available output bytes from the process (using Scanner to grab all tokens at once) and prints all the output from the process (i.e. PrintSystemDictionaryAtExit, and there is a lot of it) at failure.
>>> 
>>> Okay but that's the rest of the output that hasn't yet been processed - correct? So the line of output that caused the failure won't be there as it's already been grabbed by the scanner and processed.
>> Good catch, I changed the fix to make sure we print all the output.
>>>>> 
>>>>> What I wanted to see was the complete history of lines of the form
>>>>> 
>>>>> "Java dictionary (table_size=107, classes=6)"
>>>>> 
>>>>> so that we can see how the table size and number of classes have changed leading up to the failure. But I realize now that we already get that from the:
>>>>> 
>>>>> System.out.println("PASS table_size:"+table_size+", classes:"+classes+" OK");
>>>>> 
>>>>> All that is missing is the printing of the table_size and classes when we throw the exception. That can be trivially fixed:
>>>>> 
>>>>>         if (loadFactor > MAX_LOAD_FACTOR) {
>>>>> +          System.out.println("FAIL table_size:"+table_size+", classes:"+classes);
>>>>>           throw new RuntimeException("Load factor too high, expected MAX "+MAX_LOAD_FACTOR+", got "+loadFactor);
>>>>>         } else {
>>>>>           System.out.println("PASS table_size:"+table_size+", classes:"+classes+" OK");
>>>>>         }
>>>> OK, so you determined that only that one piece of info was missing in the end in your case, but maybe the next person might want to see everything that PrintSystemDictionaryAtExit provides?
>>>> As long as we’re touching the test - should we then stick with the solution that prints out everything from PrintSystemDictionaryAtExit?
>>> 
>>> I'm not sure how anything that comes later in PrintSystemDictionaryAtExit would help with a failure that happened earlier, but if you see value it then that's fine. However I think you need my suggestion as well to get the information that actually caused the failure.
>> We now print all the output from PrintSystemDictionaryAtExit, but 
> 
> So you'll print all the good reports for the "Java dictionary ..." lines then if there is a failure print the entire output. That's okay - though as I mention in the bug report it does tend to hit the jtreg output buffer limit.

We only print the dictionary in case of failure, i.e:

  87         if (loadFactor > MAX_LOAD_FACTOR) {


> 
>> also following your suggestion we summarize the failure details in
>> the exception itself like:
>> java.lang.RuntimeException: Load factor too high, expected MAX 5.0, got 467.30841121495325 [table size 107, number of clases 50002]
> 
> 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?


> 
>>> Some sample output in the bug report would be useful.
>> I attached a sample output to the issue as requested.
> 
> Thanks.
> 
>> https://bugs.openjdk.java.net/browse/JDK-8202360
>> http://cr.openjdk.java.net/~gziemski/8202360_rev2
> 
> A few suggestions:
> 
>  73     String string_out = "";
>  74     while (line != null) {
>  75       string_out += line+"\n";
> 
> As this is not in fact a constant I would suggest using a (large-size) StringBuilder directly and avoid all the implicit StringBuilder and String usage that the '+' operator will hide. The variable can just be called 'output’.

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.


> 88           System.out.println(string_out);
> 
> Please precede with a comment
> 
> // We've hit an error so print all of the output parsed so far, and
> // then the remaining output.

Done.


> 
> 93           throw new RuntimeException("Load factor too high, expected MAX "+MAX_LOAD_FACTOR+", got "+loadFactor+" [table size "+table_size+", number of clases "+classes+"]");
> 
> 
> Please split this line up to make it much shorter. I'm tempted to ask you to fix this missing spaces around the '+' operator but that bad-style is prevalent throughout this test. (This violates a number of Java style rules.)

Done and done.


> 
> Don't forget to add second copyright year.

Done.

After all the feedback I like the code much better now. Thank you for thorough review and the detailed feedback.

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


cheers