Skip to content

Conversation

@Technici4n
Copy link
Member

Depends on McModLauncher/securejarhandler#47.

The IModFileInfo creation is still quite messy, but that will be for another time. :P

);
var jarContents = new JarContentsBuilder()
.paths(path)
.ignoreRootPackages("assets", "data")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this skips scanning the assets and data folders of mods for classes... :)

@Technici4n Technici4n marked this pull request as ready for review November 27, 2023 17:43
Comment on lines 98 to 99
JarMetadata metadata = JarMetadata.from(new JarContentsBuilder().paths(path).build());
metadata.providers().stream()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This greatly speeds up scanning for providers because we are not computing the packages anymore.

}
try (var walk = Files.walk(modsDir, 1)){
walk.forEach(ModDirTransformerDiscoverer::visitFile);
walk.parallel().forEach(ModDirTransformerDiscoverer::visitFile);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walk in parallel to open file systems in parallel, should save some time too based on ATM9 profiles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work (i.e. does Files.walk's stream actually support parallelism, or is it a no-op)? Usually I like to collect into a list first and then parallel stream that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this does not work because of a JDK bug that was only fixed in Java 19: https://bugs.openjdk.org/browse/JDK-8280915. Thanks for noticing...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7a0fecd.

@Technici4n
Copy link
Member Author

Tested working with a few mods + a custom no-op IModLocator JAR to make sure that everything still works.

@Matyrobbrt Matyrobbrt merged commit ae8840b into neoforged:main Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants