Please reuse Objenesis instance to reduce class loading/unloading

[selckin]

Instantiator creates a new instance of Objenesis for every instance it creates. Objenesis is made to be reused and caches the loaded classes inside, so the jvm does not need to recreate them every time.

public class Main {
    public static void main(String[] args) {
        IntStream.range(0, 5000000).parallel().forEach(value -> {
            System.out.println("tick " + value);
            EqualsVerifier.forClass(Foo.class).withNonnullFields("testId", "result").verify();
        });
    }
}

public final class Foo {
    private final String testId;
    private final Status result;

    public Foo(String testId, Status result) {
        this.testId = requireNonNull(testId);
        this.result = requireNonNull(result);
    }
 ... removed
}

The above example will continuously create and load new classes in the jvm, which then have to be unloaded.
After some time this can even take the jvm down because it fully freezes/locks for 30+ seconds while unloading classes.
[43,321s][debug][gc,phases ] GC(1) ClassLoaderData 34830,025ms
If you monitor the classes for example in visualvm, you will also see it creating 20-40k classes continuously and struggle to gc them.

The locking/freezing can be seen as a jvm bug however with this change to reuse objenesis no more then 3k classes total are ever created in this example.

This was found while investigating why https://github.com/junit-pioneer/junit-pioneer froze while running tests on repeat to detect concurrency issues.

[jqno]

Hi!

Thanks so much for submitting this, I've been meaning to profile EqualsVerifier for a long time but never really got around to it, so I'm very happy with your PR!

Before I merge, I have a question though. The JavaDoc for ObjenesisHelper says "It is strongly not recommended to use this class", without really explaining why it's not recommended. I could, of course, make a static reference to ObjenesisStd in my own code, but I assume that's not recommended for the same reasons, whatever they may be.

We can also change the code such that only 1 instance of Objenesis is constructed per run of EqualsVerifier, but then obviously we only get partial gains.

What do you think is the best way to go here? Did you have reasons for choosing ObjenesisHelper despite this warning, or are you as in the dark about it as I am?

[selckin]

They recommend that probably because its a global static and no way to clean it out.
I used that because its the "default" global static to be used, instead of creating your own. (but since the library is shaded in, it will be your own anyway)

This library does not seem to have lifecycles where you can do any cleanup, or any way to reuse EqualsVerifier instances (all mutable and not thread safe). I didn't look too deep so maybe that is wrong, so there doesn't seem to be a place to keep shared state around in your library, so a global static the only option I see without API changes to make that possible.

The "don't use this" reasons for ObjenesisHelper don't really apply for something not designed to be used in a long running application in my opinion, This library is mainly for usage in tests i think? where they are expected to terminate quite fast. It would require huge amounts of dynamic classes to be created at runtime and then tested by your library to even notice the cache filling up. And nobody is going to be mocking your library like the other reason they give.
In the worst case you could maybe build something to query the objenesis cache size and flush occasionally.

So the options are to

1. use a static
2. push the responsibility to the user to provide a shared space/object to store cached data if they want performance.

option 1 "just works", option 2 is then advanced feature to avoid using the static that needs to be documented, maybe a EqualsVerifier.createContext() method, and then maybe something EqualsVerifier.forClass(Foo.class, context), and then the user can optionally decide to use it or not.

Another way to do option 2 would be to introduce some sort of threadsafe factory for EqualsVerifier, and then store the cache in the factory, a bit more intuitive to the user that the factory can keep state

I saw a bunch of other caches in the code, maybe some of those can also benefit from being shared, anyway i looked at your code maybe 10min, so i trust you can judge the best solution more then me :)

[jqno]

Thanks for the elaborate reply. I agree with your reasoning: let's use the ObjenesisHelper, at least for now. I can always make further improvements later. Indeed, EqualsVerifier is intended to be run only in tests, so it shouldn't pose too much of a problem.
The Objenesis instance (and indeed, as you mention, the other caches) can always be refactored out into a separate object that either the user manages, or EqualsVerifier does something smart with.

[jqno]

I've just made a release with your PR: version 3.5.5.

[hisener]

Would it make sense to use ObjenesisHelper#getInstantiatorOf in the constructor and re-use the ObjectInstantiator instance as Objenesis doc states:

To improve performance, it is best to reuse the ObjectInstantiator objects as much as possible. For example, if you are instantiating multiple instances of a specific class, do it from the same ObjectInstantiator.
http://objenesis.org/tutorial.html

Probably not:

ObjenesisBase provides a built-in cache that is activated by default. This cache keeps ObjectInstantiator instances per Class. This improves performance quite a lot when the same Class is frequently instantiated. For this reason, it is recommended to reuse Objenesis instances whenever possibly or to keep is as a singleton in your favorite dependency injection framework.
http://objenesis.org/details.html

[jqno]

Perhaps. I should look into this a little more at some point. The thing that's complicated, of course, is the fact that many users have multiple calls to EqualsVerifier in their code base (I introduced #forPackage() to mitigate that, but it's a relatively recent thing), so that would mean that they are going to have to manage the Objenesis instance (whether directly, or wrapped inside some EqualsVerifier context class), and I'd prefer not to if it's not really necessary. So far I haven't had many complaints about performance, which seems to indicate that it's ok for most users. I hope this PR will be sufficient to help power users such as yourselves :). But if not, there certainly is room for improvement still.