On Fri, 22 Mar 2024 09:25:06 GMT, Viktor Klang <vkl...@openjdk.org> wrote:
>> Benchmark Mode Cnt Score Error Units >> ImmutableColls.forEachOverList thrpt 15 361.423 ± 8.751 ops/us >> ImmutableColls.forEachOverSet thrpt 15 79.158 ± 5.064 ops/us >> ImmutableColls.getOrDefault thrpt 15 244.012 ± 0.943 ops/us >> ImmutableColls.iterateOverList thrpt 15 152.598 ± 3.687 ops/us >> ImmutableColls.iterateOverSet thrpt 15 61.969 ± 4.453 ops/us >> >> The 3 results are also available at >> https://gist.github.com/f0b4336e5b1cf9c5299ebdbcd82232bf, where baseline is >> the master this patch currently is based on (which has WhiteBoxResizeTest >> failures), patch-0 being the current code, and patch-1 being your proposal >> (uncommited patch below). >> >> diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java >> b/src/java.base/share/classes/java/util/ImmutableCollections.java >> index fc232a521fb..f38b093cf60 100644 >> --- a/src/java.base/share/classes/java/util/ImmutableCollections.java >> +++ b/src/java.base/share/classes/java/util/ImmutableCollections.java >> @@ -916,12 +916,9 @@ public <T> T[] toArray(T[] a) { >> @Override >> @SuppressWarnings("unchecked") >> public void forEach(Consumer<? super E> action) { >> - if (e1 == EMPTY) { >> - action.accept(e0); // implicit null check >> - } else { >> - action.accept(REVERSE ? (E)e1 : e0); // implicit null check >> - action.accept(REVERSE ? e0 : (E)e1); >> - } >> + action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E) e1); // >> implicit null check >> + if (e1 != EMPTY) >> + action.accept(!REVERSE ? (E) e1 : e0); >> } >> >> @Override >> >> >> >> My testing shows that the existing version I have is most likely faster than >> your proposed version. >> >> Also note that the test failures are from WhiteBoxResizeTest that's fixed in >> latest master; I decide not to pull as not to invalidate the existing >> benchmark baselines. > > Thanks. I was mostly trying to gauge what the bottleneck might be. Another alternative is this: if (e1 == EMPTY) { action.accept(e0); // implicit null check } else if (REVERSE) { action.accept((E)e1); // implicit null check action.accept(e0); } else { action.accept(e0); // implicit null check action.accept((E)e1); } I don't care about speed, so don't benchmark unless you're really really curious for yourself. I'm more concerned about clarity. The two ternary operators are a bit weird. My suggestion is bulkier but maybe clearer -- or maybe not. There is also the fact that the fields are `E e0` and `Object e1` which adds clutter from casting along with some unpleasant asymmetry. But that's a separate matter. Anyway I'm not so fond of my suggestion, so I won't advocate for it strenuously. Mostly just putting out for discussion. The current code is probably fine. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1775979779
