Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote: > `Map.containsKey` call is sometimes unnecessary, when it's known that Map > doesn't contain `null` values. > Instead we can just use Map.get and compare result with `null`. > I found one of such place, where Map.containsKey calls could be eliminated - > `java.time.format.ZoneName`. > it gives a bit of performance for `toZid`. > > > Benchmark Mode Cnt Score Error Units > ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op > ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op > > > > Benchmark > > > @BenchmarkMode(value = Mode.AverageTime) > @OutputTimeUnit(TimeUnit.NANOSECONDS) > @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) > @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) > @Fork(1) > @State(Scope.Benchmark) > public class ZoneNamesBench { > > @Benchmark > public String useExistingCountry() { > return ZoneName.toZid("Africa/Tunis", Locale.ITALY); > } > > @Benchmark > public String useExistingCountryOld() { > return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); > } > } > > > > public static String toZid(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null) { > String alias = aliases.get(zid); > if (alias != null) { > zid = alias; > mzone = zidToMzone.get(zid); > } > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map == null || ((zid = map.get(locale.getCountry())) == > null)) { > zid = mzoneToZid.get(mzone); > } > } > return toZid(zid); > } > > public static String toZid(String zid) { > return aliases.getOrDefault(zid, zid); > } > > public static String toZidOld(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null && aliases.containsKey(zid)) { > zid = aliases.get(zid); > mzone = zidToMzone.get(zid); > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map != null && map.containsKey(locale.getCountry())) { > zid = map.get(locale.getCountry()); > } else { > zid = mzoneToZid.get(mzone); > } > } > return toZidOld(zid); > } > > public static String toZidOld(String zid) { > if (aliases.containsKey(zid)) { > return aliases.get(zid); > } > return zid; > } > > Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote: > `Map.containsKey` call is sometimes unnecessary, when it's known that Map > doesn't contain `null` values. > Instead we can just use Map.get and compare result with `null`. > I found one of such place, where Map.containsKey calls could be eliminated - > `java.time.format.ZoneName`. > it gives a bit of performance for `toZid`. > > > Benchmark Mode Cnt Score Error Units > ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op > ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op > > > > Benchmark > > > @BenchmarkMode(value = Mode.AverageTime) > @OutputTimeUnit(TimeUnit.NANOSECONDS) > @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) > @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) > @Fork(1) > @State(Scope.Benchmark) > public class ZoneNamesBench { > > @Benchmark > public String useExistingCountry() { > return ZoneName.toZid("Africa/Tunis", Locale.ITALY); > } > > @Benchmark > public String useExistingCountryOld() { > return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); > } > } > > > > public static String toZid(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null) { > String alias = aliases.get(zid); > if (alias != null) { > zid = alias; > mzone = zidToMzone.get(zid); > } > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map == null || ((zid = map.get(locale.getCountry())) == > null)) { > zid = mzoneToZid.get(mzone); > } > } > return toZid(zid); > } > > public static String toZid(String zid) { > return aliases.getOrDefault(zid, zid); > } > > public static String toZidOld(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null && aliases.containsKey(zid)) { > zid = aliases.get(zid); > mzone = zidToMzone.get(zid); > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map != null && map.containsKey(locale.getCountry())) { > zid = map.get(locale.getCountry()); > } else { > zid = mzoneToZid.get(mzone); > } > } > return toZidOld(zid); > } > > public static String toZidOld(String zid) { > if (aliases.containsKey(zid)) { > return aliases.get(zid); > } > return zid; > } > > Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote: > `Map.containsKey` call is sometimes unnecessary, when it's known that Map > doesn't contain `null` values. > Instead we can just use Map.get and compare result with `null`. > I found one of such place, where Map.containsKey calls could be eliminated - > `java.time.format.ZoneName`. > it gives a bit of performance for `toZid`. > > > Benchmark Mode Cnt Score Error Units > ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op > ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op > > > > Benchmark > > > @BenchmarkMode(value = Mode.AverageTime) > @OutputTimeUnit(TimeUnit.NANOSECONDS) > @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) > @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) > @Fork(1) > @State(Scope.Benchmark) > public class ZoneNamesBench { > > @Benchmark > public String useExistingCountry() { > return ZoneName.toZid("Africa/Tunis", Locale.ITALY); > } > > @Benchmark > public String useExistingCountryOld() { > return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); > } > } > > > > public static String toZid(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null) { > String alias = aliases.get(zid); > if (alias != null) { > zid = alias; > mzone = zidToMzone.get(zid); > } > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map == null || ((zid = map.get(locale.getCountry())) == > null)) { > zid = mzoneToZid.get(mzone); > } > } > return toZid(zid); > } > > public static String toZid(String zid) { > return aliases.getOrDefault(zid, zid); > } > > public static String toZidOld(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null && aliases.containsKey(zid)) { > zid = aliases.get(zid); > mzone = zidToMzone.get(zid); > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map != null && map.containsKey(locale.getCountry())) { > zid = map.get(locale.getCountry()); > } else { > zid = mzoneToZid.get(mzone); > } > } > return toZidOld(zid); > } > > public static String toZidOld(String zid) { > if (aliases.containsKey(zid)) { > return aliases.get(zid); > } > return zid; > } > > Logically, this LGTM - Marked as reviewed by scolebourne (Author). PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote: > `Map.containsKey` call is sometimes unnecessary, when it's known that Map > doesn't contain `null` values. > Instead we can just use Map.get and compare result with `null`. > I found one of such place, where Map.containsKey calls could be eliminated - > `java.time.format.ZoneName`. > it gives a bit of performance for `toZid`. > > > Benchmark Mode Cnt Score Error Units > ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op > ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op > > > > Benchmark > > > @BenchmarkMode(value = Mode.AverageTime) > @OutputTimeUnit(TimeUnit.NANOSECONDS) > @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) > @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) > @Fork(1) > @State(Scope.Benchmark) > public class ZoneNamesBench { > > @Benchmark > public String useExistingCountry() { > return ZoneName.toZid("Africa/Tunis", Locale.ITALY); > } > > @Benchmark > public String useExistingCountryOld() { > return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); > } > } > > > > public static String toZid(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null) { > String alias = aliases.get(zid); > if (alias != null) { > zid = alias; > mzone = zidToMzone.get(zid); > } > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map == null || ((zid = map.get(locale.getCountry())) == > null)) { > zid = mzoneToZid.get(mzone); > } > } > return toZid(zid); > } > > public static String toZid(String zid) { > return aliases.getOrDefault(zid, zid); > } > > public static String toZidOld(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null && aliases.containsKey(zid)) { > zid = aliases.get(zid); > mzone = zidToMzone.get(zid); > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map != null && map.containsKey(locale.getCountry())) { > zid = map.get(locale.getCountry()); > } else { > zid = mzoneToZid.get(mzone); > } > } > return toZidOld(zid); > } > > public static String toZidOld(String zid) { > if (aliases.containsKey(zid)) { > return aliases.get(zid); > } > return zid; > } > > Hm, build of this branch fails with Crash on `Optimizing the exploded image` on one of my machines. 樂 EXCEPTION_ACCESS_VIOLATION (0xc005) at pc=0x, pid=57300, tid=10552 [hs_err_pid49380.log](https://github.com/openjdk/jdk/files/8594756/hs_err_pid49380.log) [hs_err_pid57300.log](https://github.com/openjdk/jdk/files/8594758/hs_err_pid57300.log) - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 20:45:20 GMT, Andrey Turbanov wrote: >> src/java.base/share/classes/java/time/format/ZoneName.java.template line 60: >> >>> 58: >>> 59: public static String toZid(String zid) { >>> 60: return aliases.getOrDefault(zid, zid); >> >> Is the behavior if zid == null the same? aliases.getOrDefault will throw >> NPE on null. >> neither Hashmap.containsKey or .get throw on null. > >>aliases.getOrDefault will throw NPE on null > > No, It will not. `aliases` is a HashMap. And HashMap supports null values and > keys. Anyway, this method is used only in `DateTimeFormatterBuilder` and it doesn't pass `null` value there. - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 20:36:59 GMT, Roger Riggs wrote: >aliases.getOrDefault will throw NPE on null No, It will not. `aliases` is a HashMap. And HashMap supports null values and keys. - PR: https://git.openjdk.java.net/jdk/pull/8463
Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov wrote: > `Map.containsKey` call is sometimes unnecessary, when it's known that Map > doesn't contain `null` values. > Instead we can just use Map.get and compare result with `null`. > I found one of such place, where Map.containsKey calls could be eliminated - > `java.time.format.ZoneName`. > it gives a bit of performance for `toZid`. > > > Benchmark Mode Cnt Score Error Units > ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op > ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op > > > > Benchmark > > > @BenchmarkMode(value = Mode.AverageTime) > @OutputTimeUnit(TimeUnit.NANOSECONDS) > @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) > @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) > @Fork(1) > @State(Scope.Benchmark) > public class ZoneNamesBench { > > @Benchmark > public String useExistingCountry() { > return ZoneName.toZid("Africa/Tunis", Locale.ITALY); > } > > @Benchmark > public String useExistingCountryOld() { > return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); > } > } > > > > public static String toZid(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null) { > String alias = aliases.get(zid); > if (alias != null) { > zid = alias; > mzone = zidToMzone.get(zid); > } > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map == null || ((zid = map.get(locale.getCountry())) == > null)) { > zid = mzoneToZid.get(mzone); > } > } > return toZid(zid); > } > > public static String toZid(String zid) { > return aliases.getOrDefault(zid, zid); > } > > public static String toZidOld(String zid, Locale locale) { > String mzone = zidToMzone.get(zid); > if (mzone == null && aliases.containsKey(zid)) { > zid = aliases.get(zid); > mzone = zidToMzone.get(zid); > } > if (mzone != null) { > Map map = mzoneToZidL.get(mzone); > if (map != null && map.containsKey(locale.getCountry())) { > zid = map.get(locale.getCountry()); > } else { > zid = mzoneToZid.get(mzone); > } > } > return toZidOld(zid); > } > > public static String toZidOld(String zid) { > if (aliases.containsKey(zid)) { > return aliases.get(zid); > } > return zid; > } > > src/java.base/share/classes/java/time/format/ZoneName.java.template line 60: > 58: > 59: public static String toZid(String zid) { > 60: return aliases.getOrDefault(zid, zid); Is the behavior if zid == null the same? aliases.getOrDefault will throw NPE on null. neither Hashmap.containsKey or .get throw on null. - PR: https://git.openjdk.java.net/jdk/pull/8463
RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName
`Map.containsKey` call is sometimes unnecessary, when it's known that Map doesn't contain `null` values. Instead we can just use Map.get and compare result with `null`. I found one of such place, where Map.containsKey calls could be eliminated - `java.time.format.ZoneName`. it gives a bit of performance for `toZid`. Benchmark Mode Cnt Score Error Units ZoneNamesBench.useExistingCountry avgt5 10,738 ± 0,065 ns/op ZoneNamesBench.useExistingCountryOld avgt5 13,679 ± 0,089 ns/op Benchmark @BenchmarkMode(value = Mode.AverageTime) @OutputTimeUnit(TimeUnit.NANOSECONDS) @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS) @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS) @Fork(1) @State(Scope.Benchmark) public class ZoneNamesBench { @Benchmark public String useExistingCountry() { return ZoneName.toZid("Africa/Tunis", Locale.ITALY); } @Benchmark public String useExistingCountryOld() { return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY); } } public static String toZid(String zid, Locale locale) { String mzone = zidToMzone.get(zid); if (mzone == null) { String alias = aliases.get(zid); if (alias != null) { zid = alias; mzone = zidToMzone.get(zid); } } if (mzone != null) { Map map = mzoneToZidL.get(mzone); if (map == null || ((zid = map.get(locale.getCountry())) == null)) { zid = mzoneToZid.get(mzone); } } return toZid(zid); } public static String toZid(String zid) { return aliases.getOrDefault(zid, zid); } public static String toZidOld(String zid, Locale locale) { String mzone = zidToMzone.get(zid); if (mzone == null && aliases.containsKey(zid)) { zid = aliases.get(zid); mzone = zidToMzone.get(zid); } if (mzone != null) { Map map = mzoneToZidL.get(mzone); if (map != null && map.containsKey(locale.getCountry())) { zid = map.get(locale.getCountry()); } else { zid = mzoneToZid.get(mzone); } } return toZidOld(zid); } public static String toZidOld(String zid) { if (aliases.containsKey(zid)) { return aliases.get(zid); } return zid; } - Commit messages: - [PATCH] Avoid redundant HashMap.containsKey calls in ZoneName - [PATCH] Avoid redundant HashMap.containsKey calls in ZoneName Changes: https://git.openjdk.java.net/jdk/pull/8463/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8463=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285947 Stats: 14 lines in 1 file changed: 3 ins; 5 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8463.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8463/head:pull/8463 PR: https://git.openjdk.java.net/jdk/pull/8463