Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TODO: Reevaluate include-entire-classpath #59

Open
LexManos opened this issue Jun 7, 2020 · 4 comments · May be fixed by #118
Open

TODO: Reevaluate include-entire-classpath #59

LexManos opened this issue Jun 7, 2020 · 4 comments · May be fixed by #118

Comments

@LexManos
Copy link
Member

LexManos commented Jun 7, 2020

The point of this is to make generics and type information better.
However it has a major memory cost as it reads ALL the classes in the classpath and parses out their metadata.

A better idea would be to make the metadata system lazy, but that runs into threading and race issues.

So, this is the TODO to look at it... eventually..

@jamierocks
Copy link

but that runs into threading and race issues.

If you use NIO, this shouldn't be an issue.

public static FileSystem openZip(final Path path, final boolean create) throws IOException {
    final URI uri = URI.create("jar:" + path.toUri());
    final Map<String, String> options = new HashMap<>();
    options.put("create", Boolean.toString(create));
    return FileSystems.newFileSystem(uri, options);
}

(https://github.com/CadixDev/Atlas/blob/6f6d28eb53e4b9badff05ca8fcbb9bea0bdb31b9/src/main/java/org/cadixdev/atlas/util/NIOHelper.java#L34-L39)

private FileSystem fs =  NIOHelper.openZip(Paths.get("example.jar"), false);

public byte[] getClass(final String className) throws IOException {
    final Path classPath = this.fs.getPath("/" + className.replace(".", "/") + ".class");
    if (Paths.notExists(classPath)) return null;
    return Files.readAllBytes(classPath);
}

(something like that anyhow, just threw that together on the issue)

@LexManos
Copy link
Member Author

LexManos commented Jun 7, 2020

I could of sworn last I looked into it NIO's jar FileSystem simply loaded the jar blob into memory while the file system existed. {I may be wrong will have to look again} So that doesn't help the memory issue.
As for threading, I'm not worried about reading the data. I'm more worried about FF itself.
Namely it's StructContext and other giant maps of class metadata.

@covers1624
Copy link
Contributor

Revamping loading/saving would be nice, NIO could definitely simplify it a lot. But that might have to be scoped to upstream. And no it doesn't load it into a blob iirc, it reads it once and loads the manifest, then opens a new stream for reading each entry, but i could be wrong there. I might look into threading the loading of classes during my decomp threading.

@Barteks2x
Copy link

Barteks2x commented Jun 8, 2020

Regarding NIO - For now it may be a good idea to not use it for operating on zips. I ran into some major issues on certain JDKs with it - specifically, writing anything to a Path that points to a zip filesystem using Files.write or similar will corrupt the output jar/zip file on some oracle JDKs, but not on openjdk (this is because in OpenJDK there are 2 bugs that together just happen to not cause issues - first bug is that Files.write may sometimes fail to close the stream properly in error conditions, and oracle fix ends up closing it twice under normal circumstances, and the other is that double closing an output stream into a zip file will corrupt the zip file)

@zml2008 zml2008 linked a pull request May 24, 2022 that will close this issue
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 a pull request may close this issue.

4 participants