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

[Erik Österlund]

Hi Sangheon,

Looks good.

Thanks,
/Erik

On 2017-10-09 20:14, sangheon.kim wrote:
> 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
>>>
>>
>