<i18n dev> RFR: 8300818: Reduce complexity of padding with DateTimeFormatter [v2]

[Claes Redestad]

On Sun, 22 Jan 2023 13:28:06 GMT, Sergey Tsypanov <stsypanov at openjdk.org> wrote:

>> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 2611:
>> 
>>> 2609:                     "Cannot print as output of " + len + " characters exceeds pad width of " + padWidth);
>>> 2610:             }
>>> 2611:             buf.insert(preLen, String.valueOf(padChar).repeat(padWidth - len));
>> 
>> Have you checked with a microbenchmark that this added allocation can be elided by JITs and that there's a significant speed-up with your changes? I don't have the necessary domain expertise to assert anything here but I suspect padding widths are typically short. Such as 2 or 4 (for day/month and year fields) so a micro should examine there's no regression for little to no padding. Unlike the original code you call `insert` even if `padWidth - len == 0`. This might be optimized away by JITs, but it'd be good to verify which is best.
>
> The modified code is called only when a user explicitly calls `padNext(int, char)`, i.e. if I modified the example snippet as
> 
> DateTimeFormatter dtf = new DateTimeFormatterBuilder()
>   .appendLiteral("Date:")
> //.padNext(20, ' ')
>   .append(DateTimeFormatter.ISO_DATE)
>   .toFormatter();
> String text = dtf.format(LocalDateTime.now());
> 
> there's no regression.
> 
> Right now I cannot build ad-hoc JDK with my changes and check it with JMH, so I'd convert this to draft until I can verify it

Meant that you should verify that something like this, which just add a little padding, doesn't regress with your changes: 

DateTimeFormatter dtf = new DateTimeFormatterBuilder()
    .appendLiteral("Year:")
    .padNext(5)
    .appendValue(ChronoField.YEAR)
    .toFormatter();
... 
dtf.format(LocalDateTime.now());

And similar for effectively no padding (`.padNext(4)` in the above example). As this API might often be used to ensure short 2-4 char fields are correctly padded I think it's important that we're not adding overhead to such use cases.

-------------

PR: https://git.openjdk.org/jdk/pull/12131