ikvmnet / ikvm

A Java Virtual Machine and Bytecode-to-IL Converter for .NET
Other
1.22k stars 111 forks source link

Lambda returns different object instance when it doesn't have to #342

Closed TheLastRar closed 1 year ago

TheLastRar commented 1 year ago

IKVM version : image-netcoreapp3.1-win7-x64 https://github.com/ikvmnet/ikvm/commit/29d8b854dd18d6c161e4b630e2e349f041f7530c, Build taken from this Action

When running under IKVM, the working set is 3-4GiB and the private bytes is ~7GiB When running under JVM, the working set and private bytes are both ~1GiB

The system I'm testing on has limited ram, so IKVM ends up memory constrained.

The startup time between JVM and IKVM is also significant (morose than the memory usage) IKVM has not completed startup after waiting over 15 minutes (I killed it after that) JVM completes startup in less than 10 seconds (Logging "Done")

wasabii commented 1 year ago

Startup time is what I'd be mostly concerned about. .NET and Hotspot deal with memory differently. Java eats up to the Xmx limit, which by default is like a GB. So who knows how much it could use if it wanted.

But, that said, there's probably not much I am going to do to look at this, because Minecraft isn't exactly something I'm personally interested in. Anybody else is free to volunteer on that front.

I would suggest a first step to try to be able to isolate the CPU usage down to a specific thing. And then we can get some test cases written that reproduce that thing.

TheLastRar commented 1 year ago

Startup time is what I'd be mostly concerned about. .NET and Hotspot deal with memory differently. Java eats up to the Xmx limit, which by default is like a GB. So who knows how much it could use if it wanted.

Setting Xmx to 8G with JVM does increase memory usage (working & private) to ~2GiB I had also tested IKVM on a machine with more RAM, which saw working set reach >15GiB (getting further in it's startup routine before I killed it)

I would suggest a first step to try to be able to isolate the CPU usage down to a specific thing. And then we can get some test cases written that reproduce that thing.

While I still suspect that the memory usage is of greater concern, do you have any guidance on this?

TheLastRar commented 1 year ago

I've been investigating the memory issue, and I think I've found the cause

A library used by Minecraft 1.13.2+, called DataFixerUpper, for data conversion is passed a set of rules to optimise for migrating data from older saves.

This library caches already optimised rules in a map, with the classes used to write the rules as a key. To facilitate this, each class overrides hashCode() by hashing its fields.

One of these classes stores a Supplier<>, on JVM two identical Supplier<>s have the same hash, on IKVM they don't.

This breaks the caching and results in a ~million extra entries being put into the cache map (maybe more if I left it running longer) and a lot of extra reprocessing of the same rules.

I've made a reduced test case for the Supplier<> hash issue;

import java.util.function.Supplier;

public class SupplierHash {

    public static void main(String[] args) {
        Supplier<Object> aa = SupplierHash::GiveObject;
        Supplier<Object> ab = SupplierHash::GiveObject;

        //Are different on JVM
        System.out.println(String.format("Function ref a %x, b %x", aa.hashCode(), ab.hashCode()));

        Supplier<Object> ba = GiveObjectSupplier1();
        Supplier<Object> bb = GiveObjectSupplier1();

        //Are same on JVM
        System.out.println(String.format("Passed Function a %x, b %x", ba.hashCode(), bb.hashCode()));

        Supplier<Object> ca = GiveObjectSupplier2();
        Supplier<Object> cb = GiveObjectSupplier2();

        //Are same on JVM
        System.out.println(String.format("Passed lambda a %x, b %x", ca.hashCode(), cb.hashCode()));        
    }

    public static Object GiveObject() {
        return new Object();
    }

    public static Supplier<Object> GiveObjectSupplier1() {
        return SupplierHash::GiveObject;
    }

    public static Supplier<Object> GiveObjectSupplier2() {
        return () -> new Object();
    }
}

On JVM, only the top test gives different hashes

Function ref a 1218025c, b 816f27d
Passed Function a eed1f14, b eed1f14
Passed lambda a 4c873330, b 4c873330

On IKVM, All tests give different hashes

Function ref a 14b2c3b, b 3a48e15
Passed Function a 1ea0a2f, b 13a5ba8
Passed lambda a 30d38e8, b 3770028

This was tested on the action build of commit 9b704cdba0db130bff71816132d8806024153356.

wasabii commented 1 year ago

Interesting. So it's expecting multiple calls to GiveObjectSupplier2 to return the same instance, likely.

wasabii commented 1 year ago

Yeah. Behavior not guarenteed by spec:

https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.27.4

We might be able to fix it though.

wasabii commented 1 year ago

I would open a bug against that library first, just pointing out that there's no guarentee in the spec that it does return the same instance, and thus can break on differnet JVMs than what they're testing on. And we can probably also work to optimize it.

wasabii commented 1 year ago

SE17 is more explicit:

https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.27.4

This implies that the identity of the result of evaluating a lambda expression (or, of serializing and deserializing a lambda expression) is unpredictable, and therefore identity-sensitive operations (such as reference equality (§15.21.3), object locking (§14.19), and the System.identityHashCode method) may produce different results in different implementations of the Java programming language, or even upon different lambda expression evaluations in the same implementation.