On Thu, 1 Oct 2026 20:05:55 GMT, Naoto Sato <[email protected]> wrote:
>> Fixing performance regression caused by >> [JDK-8381379](https://bugs.openjdk.org/browse/JDK-8381379). Instead of >> having each explicit time zone as an entry in the resource bundle, the >> explicit DST offsets are encoded in one entry. This avoids repeated lookups >> for missing offsets and eliminates the need for a separate cache. Also >> `SimpleDateFormat` now issues a new internal `ZoneInfo` method that won't >> clone instances on each format call. Here is the benchmark result for the >> test case in the JBS entry (on my mac): >> >> Before: >> >> New York: 303.9 B/op, 191.7 ns/op >> Vancouver: 240.0 B/op, 216.8 ns/op >> >> After: >> >> New York: 32.0 B/op, 79.5 ns/op >> Vancouver: 32.0 B/op, 97.5 ns/op >> >> These results suggest that the performance has returned to approximately its >> pre-JDK- 8381379 level. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed a typo Looks good overall, left some comments. src/java.base/share/classes/sun/util/calendar/ZoneInfo.java line 1: > 1: /* nit - copyright year update needed here and in `ZoneInfoFile`. src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java line 372: > 370: int separator = entry.indexOf('='); > 371: if (separator <= 0 || separator == entry.length() - 1) { > 372: throw new InternalError("Invalid metazone.dstoffsets > entry: " + entry); Should we do this type of validation at build time in the CLDR Converter as opposed to doing it during runtime? That way if there was truly an issue, we as developers could catch it, instead of application users running into unexpected `InternalError` at runtime. src/java.base/share/classes/sun/util/locale/provider/TimeZoneNameUtility.java line 186: > 184: * @param tzid the time zone ID > 185: */ > 186: public static String explicitDstOffset(String tzid) { Both callers of this method convert the String offset into a `ZoneOffset`. Should we just store the map as `<String, ZoneOffset>` to simplify the code so that callers don't need to perform an additional op? src/java.base/share/classes/sun/util/locale/provider/TimeZoneNameUtility.java line 187: > 185: */ > 186: public static String explicitDstOffset(String tzid) { > 187: return > explicitDstOffsets.get().get(canonicalTZID(tzid).orElse(tzid)); The new code unconditionally calls `canonicalTZID`, whose implementation always casts to `CLDRLocaleProviderAdapter` anyways. Thus I think we can remove the ternary in `initExplicitDstOffsets()`. ------------- PR Review: https://git.openjdk.org/jdk/pull/33133#pullrequestreview-5386492894 PR Review Comment: https://git.openjdk.org/jdk/pull/33133#discussion_r4161304546 PR Review Comment: https://git.openjdk.org/jdk/pull/33133#discussion_r4161259652 PR Review Comment: https://git.openjdk.org/jdk/pull/33133#discussion_r4161364211 PR Review Comment: https://git.openjdk.org/jdk/pull/33133#discussion_r4161289473
