Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Fri, 26 Apr 2024 22:11:07 GMT, Chen Liang wrote: >> Do we need additional tests or are these modifications already covered by >> the existing tests? > > @minborg I have added a test as part of Collection mother-of-all-tests to > ensure spliterator and forEach yields in the same order as iterator for > unmodifiable/immutable collections. One thing of note is that somehow `==` > for yields fail for some collections like > `unmodifiableSequencedMap(linkedHashSet).sequencedKeySet()` so I have to use > `Objects.equals` instead. @liach > One thing of note is that somehow `==` for yields fail for some collections > like `unmodifiableSequencedMap(linkedHashSet).sequencedKeySet()` so I have > to use `Objects.equals` instead. Actually, it’d be for `unmodifiableMap(…).entrySet()` and `Map.of(…).entrySet()`, as those create a new `UnmodifiableEntry` and `KeyValueHolder` on iteration respectively. `unmodifiableSequencedMap(…).sequencedKeySet()` should work with `==`. - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2081284896
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Tue, 23 Apr 2024 13:13:03 GMT, Per Minborg wrote: >> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 13 commits: >> >> - Use the improved form in forEach >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/imm-coll-stream >> - Null checks should probably be in the beginning... >> - mark implicit null checks >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/imm-coll-stream >> - Copyright year, revert changes for non-few element collections >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> feature/imm-coll-stream >> - Merge branch 'feature/imm-coll-stream' of >> https://github.com/liachmodded/jdk into feature/imm-coll-stream >> - Spliterator for 12, iterate/forEach benchmark >> - fix comments >> - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c > > Do we need additional tests or are these modifications already covered by the > existing tests? @minborg I have added a test as part of Collection mother-of-all-tests to ensure spliterator and forEach yields in the same order as iterator for unmodifiable/immutable collections. One thing of note is that somehow `==` for yields fail for some collections like `unmodifiableSequencedMap(linkedHashSet).sequencedKeySet()` so I have to use `Objects.equals` instead. - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2080179982
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Tue, 23 Apr 2024 13:13:03 GMT, Per Minborg wrote: > Do we need additional tests or are these modifications already covered by the > existing tests? Thanks for the note, upon review it seems the default method overrides aren't covered by existing Collection tests. I should add them to ensure the single/double element List/Set are tested. - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2072824367
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Thu, 21 Mar 2024 18:01:38 GMT, Chen Liang wrote: >> Please review this patch that: >> 1. Implemented `forEach` to optimize for 1 or 2 element collections. >> 2. Implemented `spliterator` to optimize for a single element. >> >> The default implementations for multiple-element immutable collections are >> fine as-is, specializing implementation doesn't provide much benefit. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 13 commits: > > - Use the improved form in forEach > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Null checks should probably be in the beginning... > - mark implicit null checks > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Copyright year, revert changes for non-few element collections > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Merge branch 'feature/imm-coll-stream' of > https://github.com/liachmodded/jdk into feature/imm-coll-stream > - Spliterator for 12, iterate/forEach benchmark > - fix comments > - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c Do we need additional tests or are these modifications already covered by the existing tests? - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2072289455
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Thu, 21 Mar 2024 18:01:38 GMT, Chen Liang wrote: >> Please review this patch that: >> 1. Implemented `forEach` to optimize for 1 or 2 element collections. >> 2. Implemented `spliterator` to optimize for a single element. >> >> The default implementations for multiple-element immutable collections are >> fine as-is, specializing implementation doesn't provide much benefit. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 13 commits: > > - Use the improved form in forEach > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Null checks should probably be in the beginning... > - mark implicit null checks > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Copyright year, revert changes for non-few element collections > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Merge branch 'feature/imm-coll-stream' of > https://github.com/liachmodded/jdk into feature/imm-coll-stream > - Spliterator for 12, iterate/forEach benchmark > - fix comments > - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c @stuart-marks & @AlanBateman Specialization of forEach() and spliterator()? - PR Comment: https://git.openjdk.org/jdk/pull/15834#issuecomment-2072240066
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Fri, 22 Mar 2024 00:21:56 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 924:
>>
>>> 922: action.accept(REVERSE ? (E)e1 : e0); // implicit null
>>> check
>>> 923: action.accept(REVERSE ? e0 : (E)e1);
>>> 924: }
>>
>> Out of curiosity, how does the following fare performance-wise?
>>
>> Suggestion:
>>
>> action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E)e1); //
>> implicit null check
>> if (e1 != EMPTY)
>> action.accept(!REVERSE ? (E)e1 : e0);
>
> BenchmarkMode CntScore 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[] toArray(T[] a) {
> @Override
> @SuppressWarnings("unchecked")
> public void forEach(Consumer 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1535286326
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Thu, 21 Mar 2024 20:09:23 GMT, Viktor Klang wrote:
>> Chen Liang has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 13 commits:
>>
>> - Use the improved form in forEach
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Null checks should probably be in the beginning...
>> - mark implicit null checks
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Copyright year, revert changes for non-few element collections
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Merge branch 'feature/imm-coll-stream' of
>> https://github.com/liachmodded/jdk into feature/imm-coll-stream
>> - Spliterator for 12, iterate/forEach benchmark
>> - fix comments
>> - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 924:
>
>> 922: action.accept(REVERSE ? (E)e1 : e0); // implicit null
>> check
>> 923: action.accept(REVERSE ? e0 : (E)e1);
>> 924: }
>
> Out of curiosity, how does the following fare performance-wise?
>
> Suggestion:
>
> action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E)e1); //
> implicit null check
> if (e1 != EMPTY)
> action.accept(!REVERSE ? (E)e1 : e0);
BenchmarkMode CntScore 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[] toArray(T[] a) {
@Override
@SuppressWarnings("unchecked")
public void forEach(Consumer 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1534886983
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Thu, 21 Mar 2024 18:01:38 GMT, Chen Liang wrote: >> Please review this patch that: >> 1. Implemented `forEach` to optimize for 1 or 2 element collections. >> 2. Implemented `spliterator` to optimize for a single element. >> >> The default implementations for multiple-element immutable collections are >> fine as-is, specializing implementation doesn't provide much benefit. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 13 commits: > > - Use the improved form in forEach > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Null checks should probably be in the beginning... > - mark implicit null checks > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Copyright year, revert changes for non-few element collections > - Merge branch 'master' of https://github.com/openjdk/jdk into > feature/imm-coll-stream > - Merge branch 'feature/imm-coll-stream' of > https://github.com/liachmodded/jdk into feature/imm-coll-stream > - Spliterator for 12, iterate/forEach benchmark > - fix comments > - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c src/java.base/share/classes/java/util/ImmutableCollections.java line 924: > 922: action.accept(REVERSE ? (E)e1 : e0); // implicit null > check > 923: action.accept(REVERSE ? e0 : (E)e1); > 924: } Out of curiosity, how does the following fare performance-wise? Suggestion: action.accept((!REVERSE || e1 == EMPTY) ? e0 : (E)e1); // implicit null check if (e1 != EMPTY) action.accept(!REVERSE ? (E)e1 : e0); - PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1534612528
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
On Tue, 12 Mar 2024 10:12:18 GMT, Viktor Klang wrote:
>> Chen Liang has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 13 commits:
>>
>> - Use the improved form in forEach
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Null checks should probably be in the beginning...
>> - mark implicit null checks
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Copyright year, revert changes for non-few element collections
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> feature/imm-coll-stream
>> - Merge branch 'feature/imm-coll-stream' of
>> https://github.com/liachmodded/jdk into feature/imm-coll-stream
>> - Spliterator for 12, iterate/forEach benchmark
>> - fix comments
>> - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 926:
>
>> 924: if (!REVERSE && e1 != EMPTY) {
>> 925: action.accept((E) e1);
>> 926: }
>
> I'm curious to know how the following alternative would fare:
>
> Suggestion:
>
> if (e1 != EMPTY) {
> action.accept(REVERSE ? (E)e1 : (E)e0); // implicit null check
> action.accept(REVERSE ? (E)e0 : (E)e1);
> } else {
> action.accept(e0); // Implicit null check
> }
@viktorklang-ora I've updated this piece of code, does it look better now?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15834#discussion_r1534539452
Re: RFR: 8327858: Improve spliterator and forEach for single-element immutable collections [v2]
> Please review this patch that: > 1. Implemented `forEach` to optimize for 1 or 2 element collections. > 2. Implemented `spliterator` to optimize for a single element. > > The default implementations for multiple-element immutable collections are > fine as-is, specializing implementation doesn't provide much benefit. Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Use the improved form in forEach - Merge branch 'master' of https://github.com/openjdk/jdk into feature/imm-coll-stream - Null checks should probably be in the beginning... - mark implicit null checks - Merge branch 'master' of https://github.com/openjdk/jdk into feature/imm-coll-stream - Copyright year, revert changes for non-few element collections - Merge branch 'master' of https://github.com/openjdk/jdk into feature/imm-coll-stream - Merge branch 'feature/imm-coll-stream' of https://github.com/liachmodded/jdk into feature/imm-coll-stream - Spliterator for 12, iterate/forEach benchmark - fix comments - ... and 3 more: https://git.openjdk.org/jdk/compare/d5b95a0e...69bd0e9c - Changes: https://git.openjdk.org/jdk/pull/15834/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15834=01 Stats: 89 lines in 2 files changed: 87 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15834.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15834/head:pull/15834 PR: https://git.openjdk.org/jdk/pull/15834
