On Sun, 22 Jan 2023 11:36:34 GMT, Claes Redestad <[email protected]> wrote:
>> Sergey Tsypanov has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains two additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into dtfb
>> - Improve padding of DateTimeFormatter
>
> 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
-------------
PR: https://git.openjdk.org/jdk/pull/12131
