[ji] lazy loading of Java (proxy) class extensions #5161

kares · 2018-05-10T13:56:36Z

seems to work fine to the limited testing that I did locally
the gist is - Java extensions load on demand as Java classes are (first) used
theoretically we could also add a flag to disable the laziness and behave as before

also ... we could safely add more extensions now and do not have to worry much about bloating load times

did not run any numbers yet, but hopefully we do not do much JI internally on gem list, rake etc

headius · 2018-05-13T18:16:34Z

No prob, will review. Have you managed to measure anything yet?

kares · 2018-05-14T07:07:55Z

quick test of time bin/jruby -S gem list

# BEFORE:
 
real	0m1.823s
user	0m2.945s
sys	0m0.157s

real	0m1.778s
user	0m2.951s
sys	0m0.162s

real	0m1.769s
user	0m2.884s
sys	0m0.171s

AVG 1.79s


# AFTER 

real	0m1.739s
user	0m2.780s
sys	0m0.165s

real	0m1.736s
user	0m2.847s
sys	0m0.144s

real	0m1.712s
user	0m2.764s
sys	0m0.186s

AVG 1.73s

... gets us around 5% on my machine

headius

The static reference needs to be fixed, and the naming is problematic, but otherwise this seems ok. I worry a bit about races but the logic that sets up the proxy class should be safe currently. Will request review from @enebo.

headius · 2018-05-14T20:42:27Z

core/src/main/java/org/jruby/javasupport/ext/JavaLang.java

    }

    @JRubyModule(name = "Java::JavaLang::Iterable", include = "Enumerable")
    public static class Iterable {

-        static RubyModule define(final Ruby runtime) {
-            final RubyModule Iterable = Java.getProxyClass(runtime, java.lang.Iterable.class);
+        static RubyModule define(final Ruby runtime, final RubyModule Iterable) {


Could we use a different name for these? I find it extremely confusing when I look at the code and I see "Iterable" meaning two different things.

Note that at this point in the code, Iterable now means three different things: the inner class, the variable, and (to me at a glance) the actual Java Iterable class. It's quite confusing.

Same comment applies to all other such names below. I understand that it might look a bit nicer to some, but this flies against typical Java local variable naming convention.

It's somewhat confusing that the extension classes have the same names as the Java class they extend, but that seems like less of an issue to me than the local variables having yet again the same name.

okay, will take a look into that

headius · 2018-05-14T20:51:39Z

core/src/main/java/org/jruby/javasupport/ext/JavaExtensions.java

+
+    private static final boolean IMMEDIATE = false;
+
+    private static final JavaExtensions INSTANCE = new JavaExtensions();


This should not be static; a better place would be for it to live on the javasupport Java instance attached to the runtime.

It is weak on the value, but it also anchors a class as its key. The classes we currently extend are system-level classes, but this may be used in the future for classes loaded as part of JRuby or within extensions (e.g. jruby-openssl) that load into their own classloaders, and this reference will prevent them from being collected.

It also is not compatible with multiple JRuby instances in the same JVM, since only the first one will register (and eventually apply) an extension. The others will race to insert the extension or remove it when applying that extension.

Oh somehow my review used an older version that was still static. I see you have fixed that, thanks!

kares · 2018-05-15T14:29:30Z

The static reference needs to be fixed, and the naming is problematic, but otherwise this seems ok.

✔️

I worry a bit about races but the logic that sets up the proxy class should be safe currently

exactly - its plugged in right around proxy-class initialization which is already race condition tuned (some tests are in place)

enebo · 2018-05-18T14:07:30Z

core/src/main/java/org/jruby/javasupport/ext/JavaExtensions.java

+        assert previous == null;
+    }
+
+    public static void define(final Class javaClass, final RubyModule proxyClass) {


Seems like runtime could be a parameter here to match put().

It also seems to line up more with Iterable stuff below which also have runtime.

enebo

Seems reasonable to me. I guess only fear is proxy class initialization concurrency, but that has not actually changed at all in this PR so seems fine.

kares added this to the JRuby 9.2.0.0 milestone May 13, 2018

kares requested a review from headius May 13, 2018 17:50

headius requested changes May 14, 2018

View reviewed changes

headius requested a review from enebo May 14, 2018 20:55

kares added 2 commits May 18, 2018 11:26

[ji] lazy loading Java (proxy) class extensions as they're being used

5339f60

of course, need to track Java extension loading per runtime

bd96565

enebo reviewed May 18, 2018

View reviewed changes

enebo approved these changes May 18, 2018

View reviewed changes

kares force-pushed the ji-lazy branch 2 times, most recently from 3325389 to ab086b7 Compare May 18, 2018 14:45

kares added 2 commits May 18, 2018 16:45

[ji] its for the best to have -Xji.load.lazy=false switch available

b02bb2c

[refactor] less confusing proxy class/module local variable name

ab086b7

kares merged commit 262bea3 into master May 18, 2018

kares deleted the ji-lazy branch May 18, 2018 14:59

[ji] lazy loading of Java (proxy) class extensions #5161

[ji] lazy loading of Java (proxy) class extensions #5161

kares commented May 10, 2018

headius commented May 13, 2018

kares commented May 14, 2018

headius left a comment

headius May 14, 2018

headius May 14, 2018

headius May 14, 2018

kares May 15, 2018

headius May 14, 2018

headius May 14, 2018

kares commented May 15, 2018

enebo May 18, 2018

enebo May 18, 2018

enebo left a comment


		private static final boolean IMMEDIATE = false;

		private static final JavaExtensions INSTANCE = new JavaExtensions();

[ji] lazy loading of Java (proxy) class extensions #5161

[ji] lazy loading of Java (proxy) class extensions #5161

Conversation

kares commented May 10, 2018

headius commented May 13, 2018

kares commented May 14, 2018

headius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kares commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enebo left a comment

Choose a reason for hiding this comment