Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad wrote: >> The commentary on this line could probably be improved, but this is in a >> private printer-parser that will only be used for NANO_OF_SECOND and not any >> arbitrary `TemporalField` (see line 704), thus I fail to see how this >> assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to >> 999,999,999). >> >> I considered writing a more generic integral-fraction printer parser that >> would optimize for any value-range that fits in an int, but seeing how >> NANO_OF_SECOND is likely the only one used in practice and with a high >> demand for better efficiency I opted to specialize for it more directly. > > I see what you're saying that an arbitrary `Temporal` could define its own > fields with its own ranges, but I would consider it a design bug if such an > implementation at a whim redefines the value ranges of well-defined constants > such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a > `Temporal` would have to define its own enumeration of allowed > `TemporalField`s. That isn't the design model however. The design model for the formatter is a `Map` like view of field to value. Any value may be associated with any field - that is exactly what `Temporal` offers. [`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/time/temporal/TemporalAccessor.html#getLong(java.time.temporal.TemporalField)) is very explicit about this. As indicated above, the positive part is that an hour-of-day of 26 can be printed by a user-written `WrappingLocalTime` class. The downside is the inability to make optimizing assumptions as per this code. FWIW, I had originally intended to write dedicated private formatters where the pattern and type to be formatted are known, such as `LocalDate` and the ISO pattern. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
On Mon, 1 Nov 2021 22:27:08 GMT, Claes Redestad wrote: >> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java >> line 3269: >> >>> 3267: return false; >>> 3268: } >>> 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in >>> an int and can't be negative >> >> Unfortunately, this is not a valid assumption, and it affects the logic of >> the optimization more generally (methods where non-negative is assumed). >> >> Basically, NANO_OF_SECOND (and all other fields in the formatter) can have >> any `long` value. Despite immediate appearances, the value is not limited to >> 0 to 999,999,999. This is because you are allowed to create an >> implementation of `Temporal` that returns values outside that range. No such >> class exists in the JDK, but such a class would be perfectly legal. As a >> related example, it would be perfectly value to write a time class that ran >> from 03:00 to 26:59 each day, thus HOUROF_DAY cannot be assumed by the >> formatter to be between 0 and 23. > > The commentary on this line could probably be improved, but this is in a > private printer-parser that will only be used for NANO_OF_SECOND and not any > arbitrary `TemporalField` (see line 704), thus I fail to see how this > assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to > 999,999,999). > > I considered writing a more generic integral-fraction printer parser that > would optimize for any value-range that fits in an int, but seeing how > NANO_OF_SECOND is likely the only one used in practice and with a high demand > for better efficiency I opted to specialize for it more directly. I see what you're saying that an arbitrary `Temporal` could define its own fields with its own ranges, but I would consider it a design bug if such an implementation at a whim redefines the value ranges of well-defined constants such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a `Temporal` would have to define its own enumeration of allowed `TemporalField`s. - PR: https://git.openjdk.java.net/jdk/pull/6188
Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]
> Prompted by a request from Volkan Yazıcı I took a look at why the java.time > formatters are less efficient for some common patterns than custom formatters > in apache-commons and log4j. This patch reduces the gap, without having > looked at the third party implementations. > > When printing times: > - Avoid turning integral values into `String`s before appending them to the > buffer > - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of > `BigDecimal` > > This means a speed-up and reduction in allocations when formatting almost any > date or time pattern, and especially so when including sub-second parts > (`S-S`). > > Much of the remaining overhead can be traced to the need to create a > `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` > internally. We could likely also win performance by specializing some common > patterns. > > Testing: tier1-3 Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove stray method - Changes: - all: https://git.openjdk.java.net/jdk/pull/6188/files - new: https://git.openjdk.java.net/jdk/pull/6188/files/9e97b4dc..ee13139a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6188=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6188=00-01 Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6188.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6188/head:pull/6188 PR: https://git.openjdk.java.net/jdk/pull/6188
