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

[Erik Gahlin]

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/ <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 <mailto: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 <mailto: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 <https://bugs.openjdk.java.net/browse/JDK-8223697>
>> > webrev http://cr.openjdk.java.net/~cito/JDK-8223697/webrev.00/ <http://cr.openjdk.java.net/%7Ecito/JDK-8223697/webrev.00/>
>> >
>> > Regards,
>> > Chihiro
>> >
>> >
>> > 2019年9月14日(土) 0:06 Chihiro Ito < <mailto:chiroito107 at gmail.com>chiroito107 at gmail.com <mailto:chiroito107 at gmail.com>>:
>> >
>> >> Hi,
>> >>
>> >> Could you review this tiny change, please?
>> >>
>> >> http://cr.openjdk.java.net/~cito/JDK-8223697/webrev.00/ <http://cr.openjdk.java.net/%7Ecito/JDK-8223697/webrev.00/>
>> >>
>> >> Regards,
>> >> Chihiro
>> >>
>> 
>