Re: RFR: 8281298: Revise the creation of unmodifiable list
On Sun, 6 Feb 2022 22:11:06 GMT, Claes Redestad wrote: > This looks like an appropriate solution which avoids the minor compatibility > risk introduced by the previous change - and you might even end up being more > efficient both when setting and reading the names/matchers. > > (Since we're going down the route of optimizing this: the `type` of a > `SNIServerName` is constrained to a value between 0 and 255, so there's > likely a small micro-optimization opportunity in using a `BitSet` for the > duplicate checking rather a `ArrayList`.. A `BitSet(256)` should > have lower memory use and the existence check will run in O(1) instead of > O(n) time. Might not be worthwhile to optimize these setters, though..) Thank you for the quick review. As the number of SNIServerName is normally one in practice, I'm not very sure of the improvement to use BitSet yet. But I'm glad to know the BitSet could be a better choice if the items number goes beyond 4. - PR: https://git.openjdk.java.net/jdk/pull/7359
Re: RFR: 8281298: Revise the creation of unmodifiable list
On Sat, 5 Feb 2022 20:29:50 GMT, Xue-Lei Andrew Fan wrote: > In [JDK-8281289](https://bugs.openjdk.java.net/browse/JDK-8281289), the > SSLParameters class was updated with the patch: > > - return Collections.unmodifiableList(new > ArrayList<>(sniNames.values())); > + return List.copyOf(sniNames.values()); > > However, there's a small compatibility risk with this change, > `List.copyOf(...).contains(null)` will throw NPE while > `Collections.unmodifiableList(...).contains(null)` won't. It may be not > worthy of the risk although the impact may be minimal. > > This update will re-use the Collections.unmodifiableList() methods, but > re-org the code for better performance. This looks like an appropriate solution which avoids the minor compatibility risk introduced by the previous change - and you might even end up being more efficient both when setting and reading the names/matchers. (Since we're going down the route of optimizing this: the `type` of a `SNIServerName` is constrained to a value between 0 and 255, so there's likely a small micro-optimization opportunity in using a `BitSet` for the duplicate checking rather a `ArrayList`.. A `BitSet(256)` should have lower memory use and the existence check will run in O(1) instead of O(n) time. Might not be worthwhile to optimize these setters, though..) - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7359
RFR: 8281298: Revise the creation of unmodifiable list
In [JDK-8281289](https://bugs.openjdk.java.net/browse/JDK-8281289), the SSLParameters class was updated with the patch: - return Collections.unmodifiableList(new ArrayList<>(sniNames.values())); + return List.copyOf(sniNames.values()); However, there's a small compatibility risk with this change, `List.copyOf(...).contains(null)` will throw NPE while `Collections.unmodifiableList(...).contains(null)` won't. It may be not worthy of the risk although the impact may be minimal. This update will re-use the Collections.unmodifiableList() methods, but re-org the code for better performance. - Commit messages: - 8281298: Revise the creation of unmodifiable list Changes: https://git.openjdk.java.net/jdk/pull/7359/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7359&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281298 Stats: 71 lines in 1 file changed: 18 ins; 25 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/7359.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7359/head:pull/7359 PR: https://git.openjdk.java.net/jdk/pull/7359
