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
