RFR (XS): 8188245: [Testbug] test/hotspot/jtreg/gc/logging/TestPrintReferences.java can fail

[sangheon.kim]

Hi Stefan,

Thanks for looking at this!

On 10/09/2017 04:26 AM, Stefan Johansson wrote:
> Hi Sangheon,
>
> Thanks for looking into this issue. I have some questions regarding 
> the fix or rather the test maybe.
>
> First, the comment for approximatelyEqual say that the max number of 
> sub-phases is 3 so we allow a tolerance of 0.1, but we also use this 
> method when summarizing all reference types and they are 4. In theory 
> the rounding for these could lead to an error of 0.2, or am I missing 
> something? This is probably not very common, but it would be nice if 
> we could make the test robust enough to not trigger such an error.
You are right in theory. For 4 sub-phases, we should allow 0.2 tolerance.
I changed as you commented.

But it will never happen under current test as we are only using 
SoftReference. i.e. To face the 0.2 tolerance case, we should have 
rounded reference time for all 4 types but we only have noticeable 
SoftReference time. But who knows someone could change the test. :)

>
> Trying to verify logging like this is hard, especially since we have 
> rounding and I'm starting to think maybe we should just verify that 
> each sub-phase is less that the parent phase and not allow any fault 
> tolerance. Doing that will do less verification so maybe we should 
> just make sure that the tolerance used take into account the number of 
> values summarized.
I'm not sure whether or not just comparing sub-phase vs. phase will 
provide enough verification.
Probably the original bug which introduce this comparison(JDK-8186465: 
Each j.l.Reference elapsed time log is incorrect) will be verified by 
your suggestion. But not sure other cases so if you don't have strong 
opinion on this, I would prefer to keep as is.

>
> Regarding the code, I would like the variables named 'total' to be 
> renamed to something more descriptive, like 'sumOfPhases', to more 
> clearly show what it is.
Done.

> Also I think it would be helpful to rename 'a' and 'b' to more 
> descriptive names. Even if the method could be generic it is only used 
> for one thing so the name could map to that.
My intent was (as you said) to keep generic name but as you commented I 
tried.

>
> So to summarize, I'm fine with the fix if we feel that we want to keep 
> this amount of verification. But I do think we need to have a variable 
> tolerance to work in both cases.
http://cr.openjdk.java.net/~sangheki/8188245/webrev.1
http://cr.openjdk.java.net/~sangheki/8188245/webrev.1_to_0/

Thanks,
Sangheon


>
> Thanks,
> Stefan
>
> On 2017-10-09 10:14, Erik Österlund wrote:
>> Hi Sangheaon,
>>
>> Looks good.
>>
>> Thanks,
>> /Erik
>>
>> On 2017-10-06 22:30, sangheon.kim wrote:
>>> Hi all,
>>>
>>> Could I have some reviews to fix Java 'double' type rounding issue?
>>>
>>> The test is trying to compare time spent at a phase and a sum of 
>>> each sub-phases, but the add and subtract operation of 'double' type 
>>> can result in rounding issue.
>>> e.g.
>>> double phaseTime=14.7;
>>> double total = 14.8;
>>> double result = phaseTime - total;
>>> // result = -0.10000000000000142 but the test is expecting -0.1 so 
>>> fails.
>>>
>>> My proposal is to use BigDecimal whenever those operations are needed.
>>>
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8188245
>>> Webrev: http://cr.openjdk.java.net/~sangheki/8188245/webrev.0/
>>> Testing: JPRT, total of 400 iterations of TestPrintReferences.java
>>>
>>> Thanks,
>>> Sangheon
>>
>