MinecraftForge / ForgeFlower

Forge's modifications to FernFlower. Fixing various bugs/inconsistencies. Main Repo: https://github.com/MinecraftForge/FernFlower
Apache License 2.0
80 stars 44 forks source link

TODO: Reevaluate include-entire-classpath #59

Open LexManos opened 4 years ago

LexManos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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)