Skip to content

Simplify file cache checks#8

Open
PaintNinja wants to merge 2 commits intoMinecraftForge:masterfrom
PaintNinja:faster-cache-checks-2
Open

Simplify file cache checks#8
PaintNinja wants to merge 2 commits intoMinecraftForge:masterfrom
PaintNinja:faster-cache-checks-2

Conversation

@PaintNinja
Copy link
Contributor

The javadocs for File#length() state that it returns 0 if the file does not exist, so the exists() check is redundant. The length was also being queried from the filesystem twice instead of stored in a local variable.

This PR fixes those things which theoretically improves performance. I've not tested this yet as FG7's fgtools appears to be hardcoded to download from the Forge maven with no means of overriding it to use mavenLocal(), which makes it a bit tedious.

Previously, it would query the FS multiple times per file on cache hit. File#length() returns 0 when it doesn't exist, so we can take advantage of that to query the FS once.
@LexManos
Copy link
Member

LexManos commented Mar 6, 2026

You should be able use maven local by adding it to the projects repos ans updating the version. https://github.com/MinecraftForge/MinecraftForge-Experimental/blob/c70e737b1ba2ccc4ed9995470dede042b7cda087/build.gradle#L60 uses the same backend code.

@Codetoil
Copy link

Codetoil commented Mar 6, 2026

I've not tested this yet as FG7's fgtools appears to be hardcoded to download from the Forge maven with no means of overriding it to use mavenLocal(), which makes it a bit tedious.

Or impossible if you don't have perms.

@PaintNinja
Copy link
Contributor Author

I don't want to publish it to Forge's maven repo every time I want to test a change locally.

@LexManos
Copy link
Member

LexManos commented Mar 6, 2026

No, what i am saying is that it SHOULD work using maven local...
It does for mavenizer...
If it doesnt for slime then ill have to look into it.
But its literally the same code that runs both. And it uses normal maven resolution. So mavenLocal() should work

@LexManos
Copy link
Member

LexManos commented Mar 7, 2026

Just verified, mavenLocal() works just fine for Slime launcher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants