On Tue, 12 May 2026 13:00:07 GMT, Per Minborg <[email protected]> wrote:
> This PR proposes to replace older `Collection.unmodifiable` wrappers with > `Set.of()` instead. This can improve performance, reduce footprint, and > reduce maintenance. There is also occasional use of `Map.of()` in nearby > places where `Set.of()` was introduced. > > The PR also contains two optimizations in `PlatformMBeanProviderImpl` that > remove `synchronized` from two methods. > > --------- > - [X] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/java.base/aix/classes/sun/nio/fs/AixFileSystem.java line 59: > 57: result.addAll(UnixFileSystem.standardFileAttributeViews()); > 58: result.add("user"); > 59: return Set.copyOf(result); Maybe have the temporary collection (`result`) be an ArrayList instead of a HashSet. Using a HashSet as the intermediate collection causes the elements to be hashed into position and deduplicated, and `Set.copyOf` hashes and deduplicates them again. Note that `standardFileAttributeViews()` returns a List in the first place. I would suggest not using the ArrayList copy constructor, since that will use a capacity that exactly matches the argument, and then adding one or two elements will force a reallocation and copying. It's probably reasonable to use the no-arg constructor (which has a default capacity of 10) which is probably large enough to accommodate the number of views of any filesystem. This comment also applies to similar code in the other filesystem classes. src/java.base/share/classes/sun/nio/ch/ServerSocketChannelImpl.java line 268: > 266: } > 267: set.addAll(ExtendedSocketOptions.serverSocketOptions()); > 268: return Set.copyOf(set); Consider using a List as the temporary collection here as well. src/java.base/share/classes/sun/security/provider/certpath/ConstraintsChecker.java line 97: > 95: supportedExts = Set.of( > 96: BasicConstraints_Id.toString(), > 97: NameConstraints_Id.toString()); Are we sure that the two elements are never duplicates? src/java.base/share/classes/sun/security/provider/certpath/KeyChecker.java line 91: > 89: KeyUsage_Id.toString(), > 90: ExtendedKeyUsage_Id.toString(), > 91: SubjectAlternativeName_Id.toString() Need to ensure that none of these are duplicates. src/java.base/share/classes/sun/security/provider/certpath/PolicyChecker.java line 160: > 158: PolicyMappings_Id.toString(), > 159: PolicyConstraints_Id.toString(), > 160: InhibitAnyPolicy_Id.toString() Duplicate checking again. src/java.base/unix/classes/sun/nio/fs/UnixFileSystem.java line 128: > 126: > 127: static List<String> standardFileAttributeViews() { > 128: return List.of("basic", "posix", "unix", "owner"); In both cases this creates a new array and a new List on every call. Is it worth stashing this in a static final? src/java.management/share/classes/java/lang/management/DefaultPlatformMBeanProvider.java line 81: > 79: return Map.of( > 80: ManagementFactory.CLASS_LOADING_MXBEAN_NAME, > 81: ManagementFactoryHelper.getClassLoadingMXBean()); In this case and in the two singleton (set) cases above, there's really no advantage of Set.of() over singleton/singletonMap. Oh... there are additional occurrences of singleton/singletonMap below to which this comment also applies. src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/signature/Reference.java line 151: > 149: Transforms.TRANSFORM_C14N_WITH_COMMENTS, > 150: Transforms.TRANSFORM_C14N11_OMIT_COMMENTS, > 151: Transforms.TRANSFORM_C14N11_WITH_COMMENTS Need to ensure none of these are duplicates. I mean, there probably aren't any duplicates, but you never know, and there's the possibility that things can change over time.... src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMCanonicalizationMethod.java line 58: > 56: CanonicalizationMethod.EXCLUSIVE_WITH_COMMENTS, > 57: DOMCanonicalXMLC14N11Method.C14N_11, > 58: DOMCanonicalXMLC14N11Method.C14N_11_WITH_COMMENTS Duplicate checking. src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/TerminalBuilder.java line 109: > 107: String.join(",", > PROP_REDIRECT_PIPE_CREATION_MODE_REFLECTION, > PROP_REDIRECT_PIPE_CREATION_MODE_NATIVE); > 108: > 109: public static final Set<String> DEPRECATED_PROVIDERS = > Set.of(PROP_PROVIDER_JNA, PROP_PROVIDER_JANSI); Not sure if we've forked JLine or are maintaining this package in sync with upstream JLine. Check with @lahodaj . src/jdk.management/share/classes/com/sun/management/internal/PlatformMBeanProviderImpl.java line 67: > 65: > 66: private List<PlatformComponent<?>> init() { > 67: List<PlatformComponent<?>> initMBeanList = new ArrayList<>(7); Not sure it's worth pre-sizing the list. Seems likely to me that it will go out of date. The list is copied eventually anyway, so this is just a temporary structure. src/jdk.management/share/classes/com/sun/management/internal/PlatformMBeanProviderImpl.java line 333: > 331: static final HotSpotDiagnostic HS_DIAG_MBEAN = new > HotSpotDiagnostic(); > 332: } > 333: return Holder.HS_DIAG_MBEAN; This change and the one below seem like they're unrelated to this bug? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245613024 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245650000 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245654556 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245657412 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245659626 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245679521 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245689457 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245702886 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245703289 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245708413 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245722566 PR Review Comment: https://git.openjdk.org/jdk/pull/31135#discussion_r3245724558
