Cache proxies of newly constructed Java objects #3581

Merged
merged 3 commits into from Jan 11, 2016

Conversation

Projects
None yet
3 participants
@smellsblue
Contributor

smellsblue commented Jan 6, 2016

This fixes #3576

I added a few related specs while I was at it (only the constructor one failed before my changes though).

I noticed the warning at spec/READ_ME_FIRST.txt, but wasn't sure if that applied to the java_integration specs, so if I should have added my spec elsewhere, please let me know.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 8, 2016

Member

Nice! I'll review tonight.

Member

headius commented Jan 8, 2016

Nice! I'll review tonight.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 8, 2016

Member

🌲 👍

Member

kares commented Jan 8, 2016

🌲 👍

@kares kares added this to the JRuby 9.0.5.0 milestone Jan 10, 2016

kares added a commit that referenced this pull request Jan 11, 2016

Merge pull request #3581 from smellsblue/cache-constructed-proxies
Cache proxies of newly constructed Java objects

@kares kares merged commit 1e2282a into jruby:master Jan 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Finally getting back to this now.

Member

headius commented Jan 15, 2016

Finally getting back to this now.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

Ok, this isn't quite right. This will unconditionally cache objects coming from all constructor calls whether the proxy cache is on or the class has been set to persistent.

If you look at InstanceMethodInvoker and trace through method.invokeDirect, after a long chain of calls (@kares we need to reduce this) it eventually gets back to Java.getInstance() which checks those flags and decides to cache or not.

Member

headius commented Jan 15, 2016

Ok, this isn't quite right. This will unconditionally cache objects coming from all constructor calls whether the proxy cache is on or the class has been set to persistent.

If you look at InstanceMethodInvoker and trace through method.invokeDirect, after a long chain of calls (@kares we need to reduce this) it eventually gets back to Java.getInstance() which checks those flags and decides to cache or not.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 15, 2016

Member

This might be ok if we modify setAndCacheProxyObject to check those same flags.

Member

headius commented Jan 15, 2016

This might be ok if we modify setAndCacheProxyObject to check those same flags.

@smellsblue

This comment has been minimized.

Show comment
Hide comment
@smellsblue

smellsblue Jan 16, 2016

Contributor

@headius thanks for taking a look! Sorry I didn't look deeply enough into the InstanceMethodInvoker path!

I'll take another pass and see if I can fix these problems, along with some more tests.

Contributor

smellsblue commented Jan 16, 2016

@headius thanks for taking a look! Sorry I didn't look deeply enough into the InstanceMethodInvoker path!

I'll take another pass and see if I can fix these problems, along with some more tests.

@smellsblue

This comment has been minimized.

Show comment
Hide comment
@smellsblue

smellsblue Jan 16, 2016

Contributor

@headius I'm wondering about something like this:

    private void setAndCacheProxyObject(ThreadContext context, JavaProxy proxy, Object object) {
        proxy.setObject(object);

        if (Java.OBJECT_PROXY_CACHE || Java.getProxyClassForObject(context.runtime, object).getCacheProxy()) {
            context.runtime.getJavaSupport().getObjectProxyCache().put(object, proxy);
        }
    }

However, I notice there is a RubyModule clazz parameter to all the call methods... is that already the proxy class I seek? I'll dig in a bit more, but if you know off-hand, that would be a big help :-)

Contributor

smellsblue commented Jan 16, 2016

@headius I'm wondering about something like this:

    private void setAndCacheProxyObject(ThreadContext context, JavaProxy proxy, Object object) {
        proxy.setObject(object);

        if (Java.OBJECT_PROXY_CACHE || Java.getProxyClassForObject(context.runtime, object).getCacheProxy()) {
            context.runtime.getJavaSupport().getObjectProxyCache().put(object, proxy);
        }
    }

However, I notice there is a RubyModule clazz parameter to all the call methods... is that already the proxy class I seek? I'll dig in a bit more, but if you know off-hand, that would be a big help :-)

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 16, 2016

Member

right, did not realize. will fix it unless @smellsblue spins up another PR.

Member

kares commented Jan 16, 2016

right, did not realize. will fix it unless @smellsblue spins up another PR.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jan 16, 2016

Member

@smellsblue I believe that clazz is the class on which we're calling new, so you should be able to check for the class proxy directly.

Member

headius commented Jan 16, 2016

@smellsblue I believe that clazz is the class on which we're calling new, so you should be able to check for the class proxy directly.

@smellsblue

This comment has been minimized.

Show comment
Hide comment
@smellsblue

smellsblue Jan 18, 2016

Contributor

@kares @headius I just created a new PR to both fix the bug and add some specs around proxy caching: #3602

@headius using the clazz parameter seems to work, so I went with it... if you think there might be a scenario where the clazz isn't the right RubyClass, please let me know, though I may need some guidance on how to check if it is the right RubyClass before fetching it.

Contributor

smellsblue commented Jan 18, 2016

@kares @headius I just created a new PR to both fix the bug and add some specs around proxy caching: #3602

@headius using the clazz parameter seems to work, so I went with it... if you think there might be a scenario where the clazz isn't the right RubyClass, please let me know, though I may need some guidance on how to check if it is the right RubyClass before fetching it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment