On Tue, 12 Sep 2023 17:23:00 GMT, 温绍锦 <d...@openjdk.org> wrote:
>> improve date toString performance, includes:
>>
>> java.util.Date.toString
>> java.util.Date.toGMTString
>> java.time.Instant.toString
>> java.time.LocalDate.toString
>> java.time.LocalDateTime.toString
>> java.time.LocalTime.toString
>
> 温绍锦 has updated the pull request incrementally with one additional commit
> since the last revision:
>
> merge from master
I think there's still too much going on in this PR, and it should probably be
split up into multiple PRs.
I think some of this is going a bit overboard. For starters I don't want to see
us adding a 1000-element lookup table for "`DecimalDigits::digitTriple`"
without significant evidence that that is worth it at application level. As has
been pointed out elsewhere lookup tables are prone to look great on
microbenchmarks but may behave poorly and even regress real world applications
by competing for cache. Any further additions needs to be justified with a
thorough examination and benchmarks that better simulate noisy real world
applications.
src/java.base/share/classes/jdk/internal/util/DecimalDigits.java line 77:
> 75:
> 76: static {
> 77: int[] digits_k = new int[1000];
I'm very skeptical that adding a 1000 element lookup table for is worthwhile.
That's a lookup table that'd span many cache lines, with plenty of cache
misses. And even _if_ it is to be considered it should be split out and
considered in isolation from this PR.
What does the 2, 1 or 0 value you put in the lowest byte signify?
-------------
Changes requested by redestad (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15658#pullrequestreview-1623337467
PR Review Comment: https://git.openjdk.org/jdk/pull/15658#discussion_r1323702300
