On Sun, 22 Jan 2023 13:28:06 GMT, Sergey Tsypanov <stsypa...@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
