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

ProxyFactory should not discard objenesis cache, thereby enabling efficient proxy instantiation [SPR-12755] #17352

Closed
spring-issuemaster opened this issue Feb 25, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Feb 25, 2015

Adrian Moos opened SPR-12755 and commented

We are using ProxyFactory to decorate Spring Beans when passing them over an api boundary to give each caller a proxy object of their own.

We have now noticed that proxy creation is quite an expensive process, taking 0.3 ms per proxy object, even though the proxies were all for the same target class in this test.

Analysis with JProfiler yielded the attached screenshot. Apparently, the proxy class is reused, but the objenesis ObjectInstantiator is recreated for every call to getProxy().

The objenesis tutorial recommends:

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. Both InstantiatorStrategy and ObjectInstantiator can be shared between multiple threads and used concurrently. They are thread safe.


Affects: 4.1.2

Attachments:

Issue Links:

  • #15223 Add ability to create proxy around classes that has no default constructor
  • #17722 ObjenesisCglibAopProxy's fallback mode triggers duplicate class definition error

Referenced from: commits 287045e

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Adrian Moos commented

Further analysis revealed that ObjenesisCglibAopProxy enables caching when constructing ObjenesisStd. However, this has no effect, as the cache lives in the ObjenesisStd instance, of which each ObjenesisCglibAopProxy has a separate instance, and a new ObjensisCglibAopProxy is created whenever ProxyFactory.getProxy() is invoked.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Adrian Moos commented

The Objenesis documentation writes:

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 posssibly or to keep is as a singleton in your favorite dependency injection framework.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Adrian Moos commented

As ObjenesisBase does not need a destruction callback, I think declaring the field ObjenesisCglibAopProxy.objenesis static is enough:

/**
 * Objenesis based extension of {@link CglibAopProxy} to create proxy instances without invoking the
 * constructor of the class.
 *
 * @author Oliver Gierke
 * @since 4.0
 */
// patched to fix #17352
@SuppressWarnings("serial")
class ObjenesisCglibAopProxy extends CglibAopProxy {

    private static final Log logger = LogFactory.getLog(ObjenesisCglibAopProxy.class);

    private static final ObjenesisStd objenesis = new ObjenesisStd(true);

    /**
     * Creates a new {@link ObjenesisCglibAopProxy} using the given {@link AdvisedSupport}.
     * 
     * @param config must not be {@literal null}.
     */
    public ObjenesisCglibAopProxy(AdvisedSupport config) {
        super(config);
    }

    @Override
    @SuppressWarnings("unchecked")
    protected Object createProxyClassAndInstance(Enhancer enhancer, Callback[] callbacks) {
        try {
            Factory factory = (Factory) objenesis.newInstance(enhancer.createClass());
            factory.setCallbacks(callbacks);
            return factory;
        } catch (ObjenesisException ex) {
            // Fallback to regular proxy construction on unsupported JVMs
            if (logger.isDebugEnabled()) {
                logger.debug("Unable to instantiate proxy using Objenesis, falling back to regular proxy construction",
                        ex);
            }
            return super.createProxyClassAndInstance(enhancer, callbacks);
        }
    }
}

With that change, proxy creation is nearly 10 times faster in my test.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 25, 2015

Juergen Hoeller commented

Good point! As of 4.2, we are using shared ObjenesisStd instances in a static field now, distinctly configured with and without caching - based on whether the CGLIB Enhancer is using caching itself (which it for example does not do in reloading scenarios).

Also, we need to deal with Groovy's class loader there where there can be multiple Class references for the same class name... Unfortunately, Objenesis caches by class name, not by Class reference, so we need to manually skip caching in such scenaros.

Due to the complications with selective caching, I'm turning this into a 4.2-only feature, exposing it to a release candidate phase first.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Juergen Hoeller commented

On a further note, this led to the creation of a SpringObjenesis implementation class in our org.springframework.objenesis package, as a variant of ObjenesisStd which uses a cache with Class keys instead of class names, hosted in Spring's ConcurrentReferenceHashMap. Otherwise, redefined Groovy classes (exposed as a different Class but with the same class name) would consistently fail since we can't easily detect that case for manual cache excludes.

All of our internal use of Objenesis goes through that SpringObjenesis class now, except for the special non-cached case in ObjenesisCglibAopProxy, determined by a ClassLoader reloadability check, which uses a shared ObjenesisStd(false) instance.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Oliver Drotbohm commented

Juergen Hoeller - Do you think this might become a candidate for back-porting to 4.1.x once it's been proven to work reliably in 4.2? We're using Spring's Objenesis support in Spring Data MongoDB, too and it would be cool to be able to benefit from the performance improvements once we upgrade to 4.1.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Juergen Hoeller commented

Ollie, the performance improvement is primarily in ObjenesisCglibAopProxy itself, and we do need our special Class-based cache lookup there. For all other purposes, a regular static ObjenesisStd(true) instance is equally fine and equally efficient in its cache lookups, as long as it does not have to deal with redefined classes at runtime.

If a backport makes sense for Spring Data's purposes, we can still consider it; I just don't expect it to make much difference. The fact that we're using our new SpringObjenesis class in MvcUriComponentsBuilder as well (as our only use of Objenesis outside of AOP) is rather a consistency effort for future-proofing, not based on present needs.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Oliver Drotbohm commented

We're using Objenesis to create lazy-loading proxies in MongoDB, so reading quite a few objects might actually benefit from the caching introduced. We're already using the ObjenesisStd instance with caching enabled but I wondered whether caching the instantiators might improve things even further as they seem to make up a huge chunk of the bottleneck in the above profiler result.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 26, 2015

Juergen Hoeller commented

The new SpringObjenesis thing does exactly the same caching as ObjenesisStd(true) though, just with a Class key instead of the class name. It wouldn't be more efficient, it would only possibly avoid mismatches in case of redefined Groovy classes. Not sure that's a problem you're actually having...

The idea behind SpringObjenesis is rather that potential future improvements in that specific class would automatically benefit all existing use of Objenesis within the codebase. That's not the case quite yet though, except for the case with redefined classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.