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

[Stefan Johansson]

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.

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.

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. 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.

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.

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
>