RFR: 8223697: jfr tool can't format duration values greater than 1 minute

[Chihiro Ito]

Hi Erik,

Thank you for reviewing this. Please sponsor this.

I understand machine-readable. I will change only human-readable.

Regards,
Chihiro

2019年10月10日(木) 11:16 Erik Gahlin <erik.gahlin at oracle.com>:

> Looks good.
>
> I can sponsor this.
>
> JSON and XML are meant to be machine-readable, so we should not modify how
> they are formatted.
>
> Erik
>
> (I will move the constants to the top of the class where other constants
> are defined. This is the convention used in other files in jdk.jfr module)
>
> On 8 Oct 2019, at 18:38, Chihiro Ito <chiroito107 at gmail.com> wrote:
>
> Hi Erik,
>
> Thank you for reviewing.
>
> I fixed the formatter strings.
>
> webrev http://cr.openjdk.java.net/~cito/JDK-8223697/webrev.01/
>
> This fix passed test-tier1 and test/jdk/jdk/jfr.
>
> If you want to take on other formatting issues as well. The tool should:
>> - round values correctly, i.e 60.6 s should become 61 s instead of 60 s.
>> - work with negative durations.
>> - handle larger units, i.e. minutes, hours and days. I don't think we
>> need weeks, months or years.  To make it easy to read, we shouldn't use
>> fractions. i.e "1 h 22 m" instead of "1.36 h"
>> The above could be done in a separate bug, but maybe more convenient to
>> do in the same bug for back porting purposes.
>
>
> I also think we should improve these. However, it should be fixed as a new
> enhancement, not as a bug this time.
> We will also need to clarify the rules of formatter strings and to
> consider whether to apply these fixes to XML and JSON.
> If these are decided, I will fix them.
>
> Regards,
> Chihiro
>
>
> 2019年10月3日(木) 1:56 Erik Gahlin <erik.gahlin at oracle.com>:
>
>> Hi Chihiro,
>>
>> Hi Erik,
>>
>> I think we should use Duration instead of fixing the expression.
>> Also, the if statement using s is complex, so I simplified it for each
>> unit.
>>
>> OK
>>
>>
>>
>> You wrote on JBS as follows, but if I modify only the expression,
>> 1234.567 seconds will be output as 1234.0. Is this correct?
>>
>> For example, Duration.ofMillis (1234567) economies "34.567. s" instead of
>> "1234.567 s"
>>
>> Over 1000 seconds, 0.1 seconds is too small. Therefore, I think even ".0"
>> is not necessary.
>>
>> I agree, we should leave out fractions on larger values, i.e.
>> format("%d",...)
>>
>> If you want to take on other formatting issues as well. The tool should:
>>
>> - round values correctly, i.e 60.6 s should become 61 s instead of 60 s.
>> - work with negative durations.
>> - handle larger units, i.e. minutes, hours and days. I don't think we
>> need weeks, months or years.  To make it easy to read, we shouldn't use
>> fractions. i.e "1 h 22 m" instead of "1.36 h"
>>
>> The above could be done in a separate bug, but maybe more convenient to
>> do in the same bug for back porting purposes. Any way you like.
>>
>> The unit could be added to the formatter string, i.e. String.format("%.3f
>> us", (double) d.toNanos() / 1_000));
>>
>> Thanks
>> Erik
>>
>>
>> Regards,
>> Chihiro
>>
>> 2019年9月27日(金) 18:32 Erik Gahlin <erik.gahlin at oracle.com>:
>>
>>> Hi Chihiro,
>>>
>>> The bug seems to be that:
>>>
>>> double s = d.toNanosPart() / 1000_000_000.0 + d.toSecondsPart();
>>>
>>> should be:
>>>
>>> double s = d.toNanosPart() / 1000_000_000.0 + d.toSeconds();
>>>
>>> The other changes is not strictly necessary. Is that correct?
>>>
>>> Thanks
>>> Erik
>>>
>>> > Hi,
>>> >
>>> > Can anyone review this?
>>> >
>>> > JBS https://bugs.openjdk.java.net/browse/JDK-8223697
>>> > webrev http://cr.openjdk.java.net/~cito/JDK-8223697/webrev.00/
>>> >
>>> > Regards,
>>> > Chihiro
>>> >
>>> >
>>> > 2019年9月14日(土) 0:06 Chihiro Ito < <chiroito107 at gmail.com>
>>> chiroito107 at gmail.com>:
>>> >
>>> >> Hi,
>>> >>
>>> >> Could you review this tiny change, please?
>>> >>
>>> >> http://cr.openjdk.java.net/~cito/JDK-8223697/webrev.00/
>>> >>
>>> >> Regards,
>>> >> Chihiro
>>> >>
>>>
>>>
>>
>