Re: DateTimeFormatter.format() uses exceptions for flow control

[Roger Riggs]

Created an issue [1] and included the patch.

Thanks, Roger

[1] https://bugs.openjdk.java.net/browse/JDK-8167618


On 10/10/2016 2:53 AM, Clément MATHIEU wrote:

On Mon, 2016-10-10 at 06:47 +1000, David Holmes wrote:

Hi David,


Please note that patches can only be accepted if they are sent
through, or hosted upon OpenJDK infrastructure. If the patch is small
enough can you send it inline in the email (attachments are often
stripped)

Here it is:

--- old/src/java.base/share/classes/java/time/format/DateTimePrintContext.java  
2016-10-09 17:01:30.326739656 +0200
+++ new/src/java.base/share/classes/java/time/format/DateTimePrintContext.java  
2016-10-09 17:01:30.228738595 +0200
@@ -302,13 +302,10 @@
   * @throws DateTimeException if the field is not available and the 
section is not optional
   */
  Long getValue(TemporalField field) {
-try {
+if (optional == 0) {
  return temporal.getLong(field);
-} catch (DateTimeException ex) {
-if (optional > 0) {
-return null;
-}
-throw ex;
+} else {
+return temporal.isSupported(field) ? temporal.getLong(field) : 
null;
  }
  }

Clément MATHIEU

Re: DateTimeFormatter.format() uses exceptions for flow control

[Clément MATHIEU]

On Mon, 2016-10-10 at 06:47 +1000, David Holmes wrote:

Hi David,

> Please note that patches can only be accepted if they are sent
> through, or hosted upon OpenJDK infrastructure. If the patch is small
> enough can you send it inline in the email (attachments are often
> stripped) 

Here it is: 

--- old/src/java.base/share/classes/java/time/format/DateTimePrintContext.java  
2016-10-09 17:01:30.326739656 +0200
+++ new/src/java.base/share/classes/java/time/format/DateTimePrintContext.java  
2016-10-09 17:01:30.228738595 +0200
@@ -302,13 +302,10 @@
  * @throws DateTimeException if the field is not available and the section 
is not optional
  */
 Long getValue(TemporalField field) {
-try {
+if (optional == 0) {
 return temporal.getLong(field);
-} catch (DateTimeException ex) {
-if (optional > 0) {
-return null;
-}
-throw ex;
+} else {
+return temporal.isSupported(field) ? temporal.getLong(field) : 
null;
 }
 }

Clément MATHIEU

Re: DateTimeFormatter.format() uses exceptions for flow control

[Stephen Colebourne]

This looks like it should be a worthwhile improvement.
Stephen

On 9 October 2016 at 16:24, Clément MATHIEU  wrote:
> Hi !
>
> I noticed that DateTimePrintContext.getValue() relies on exceptions to
> handle optionality. Using exceptions for flow control seems both
> unexpected and very costly, ie. I discovered the issue
> when  LocaleDate.format(BASIC_ISO_DATE) showed up as a heavy hitter in
> my application.
>
> Formatting a LocateDate as a "MMdd" string should take ~0.1μs but
> currently takes from ~2.5μs, stack depth = 0, to ~10μs, stack depth =
> 128 when an optional parser is defined but the optional field is not
> supported. This seems unfortunate when exceptions can easily be avoided
> by testing if the field is supported before trying to get its value.
>
> Webrev:
>
>   http://cdn.unportant.info/openjdk/dtf_exceptions/webrev.00/
>
> JMH benchmarks:
>
>   https://gist.github.com/cykl/020e1527546a1ba44b4bb3af6dc0484c
>
>
> What do you think ?
>
>
> Note: Many tests within java.time are currently broken because of
> TestNG upgrade, see https://bugs.openjdk.java.net/browse/JDK-8152944.
> Would it help if I spend some time adding the missing L suffixes ?
>
> Regards,
>
> Clément MATHIEU

Re: DateTimeFormatter.format() uses exceptions for flow control

[David Holmes]

Hi Clement,

Please note that patches can only be accepted if they are sent through, 
or hosted upon OpenJDK infrastructure. If the patch is small enough can 
you send it inline in the email (attachments are often stripped) 
otherwise you will need to find an OpenJDK Author who can host the patch 
for you on cr.openjdk.java.net


Thanks,
David

On 10/10/2016 1:24 AM, Clément MATHIEU wrote:

Hi !

I noticed that DateTimePrintContext.getValue() relies on exceptions to
handle optionality. Using exceptions for flow control seems both
unexpected and very costly, ie. I discovered the issue
when  LocaleDate.format(BASIC_ISO_DATE) showed up as a heavy hitter in
my application.

Formatting a LocateDate as a "MMdd" string should take ~0.1μs but
currently takes from ~2.5μs, stack depth = 0, to ~10μs, stack depth =
128 when an optional parser is defined but the optional field is not
supported. This seems unfortunate when exceptions can easily be avoided
by testing if the field is supported before trying to get its value.

Webrev:

  http://cdn.unportant.info/openjdk/dtf_exceptions/webrev.00/

JMH benchmarks:

  https://gist.github.com/cykl/020e1527546a1ba44b4bb3af6dc0484c


What do you think ?


Note: Many tests within java.time are currently broken because of
TestNG upgrade, see https://bugs.openjdk.java.net/browse/JDK-8152944.
Would it help if I spend some time adding the missing L suffixes ?

Regards,

Clément MATHIEU