RFR: 8314986: Module readability resolution is slow with large numbers of automatic modules

Chen Liang liach at openjdk.org
Tue Sep 26 15:01:17 UTC 2023


On Tue, 26 Sep 2023 14:19:55 GMT, Technici4n <duke at openjdk.org> wrote:

> Fixes the issue (hopefully) by resolving automatic modules and automatic module dependencies after propagation of non-automatic transitive dependencies. The module tests run.
> 
> I also added a few asserts to validate that the automatic modules don't read themselves (previously this was the case with > 1 automatic module). Should these asserts be added in more places or is this enough?
> 
> Alan also mentioned a conflict with the spec, I'm not sure which spec is being referred to. The documentation of `ResolvedModule#reads` states `A possibly-empty unmodifiable set`, implying that the set can be empty.
> 
> Finally, `Configuration#reads` states `// The sets stored in the graph are already immutable sets` but that does not seem to be true. Should something be done about this to limit allocation?
> 
> Please let me know!
> Cheers

src/java.base/share/classes/java/lang/module/Resolver.java line 1:

> 1: /*

You should update the ending copyright year of this file from 2022 to 2023.

src/java.base/share/classes/java/lang/module/Resolver.java line 621:

> 619: 
> 620:             String name = m1.name();
> 621:             Set<ResolvedModule> m1Reads = entry.getValue();

Why not just this?

m1Reads.addAll(nameToResolved.values());
m1Reads.remove(m1);

src/java.base/share/classes/java/lang/module/Resolver.java line 639:

> 637:                 parent.configurations()
> 638:                         .map(Configuration::modules)
> 639:                         .flatMap(Set::stream)

Can't this and below step be `.forEach(m1Reads::addAll)`?

src/java.base/share/classes/java/lang/module/Resolver.java line 665:

> 663:                 } else {
> 664:                     // parent configuration, already resolved
> 665:                     // TODO: does this allocate a copy of the set every time?

It doesn't. `List.copyOf` and `Set.copyOf` returns the passed immutable collection instance if input is already an immutable collection.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15926#discussion_r1337332185
PR Review Comment: https://git.openjdk.org/jdk/pull/15926#discussion_r1337342558
PR Review Comment: https://git.openjdk.org/jdk/pull/15926#discussion_r1337326631
PR Review Comment: https://git.openjdk.org/jdk/pull/15926#discussion_r1337350597


More information about the core-libs-dev mailing list