Allow JRuby 9K to run on Ruboto #4589

donv · 2017-05-05T13:53:17Z

Everything java.lang.invoke related is not
available on android, so for now revert to the old
behavior here.

This still makes some tests fail, but might suffice
to run on android.

Use jruby's ClassValue.

Instead of java.lang.ClassValue which ist not available on
android. See #4004.

Remove seemingly unused constant.

Could not find a usage for this. And since MethodHandles
is not available on android, I simply removed it for the time
being. See #4004.

Revert to old behavior, when indy disabled.

Just like in 8db25932200fb3ed7160066b184e6db2e29f1921

Do not init JITCompiler if it is not used.

See #4004.

Use jruby's ClassValue.

Instead of java.lang.ClassValue which ist not available on
android. See #4004.

Move idTest to where it is actually needed.

This of course destroys any gains of caching the idTest, but
gets rid of MethodHandle et al in RubyModule which is
important to run on android. See #4004.

Remove unused Method.

Since this will not run on android is not being used
currently anyway, just comment it out for now. See #4004.

Revert "relax seek-channel type requirement from FileChannel to SeekableByteChannel"

This reverts commit dc48f34.

No SeekableByteChannel on android. See #4004.

Work around missing java.nio libs on android.

See #4004.

Replace ClassValue with internal implementation.

Because java.lang.ClassValue is not available on android.
See #4004.

Add minimal MethodHandle implementation.

java.lang.invoke.Method* classes are not available on
android. See #4004.

Add code generation for android.

See #4004.

Move nullToNil out of the way.

To not have MethodHandles in Ruby.java as this
is not available on android. See #4004.

Guard against missing JITCompiler.

Necessry as JITCompiler will not get instantiated on android.
See #4004.

Remove unused methods getIdTest + newIdTest

Move Android logic to the Ruboto project

Removed extra white space

Remove the maven repository for dexmaker

Return ObjectIdentityInvalidator if indy disabled. See #4004. Everything java.lang.invoke related is not available on android, so for now revert to the old behavior here. This still makes some tests fail, but might suffice to run on android. Use jruby's ClassValue. Instead of java.lang.ClassValue which ist not available on android. See #4004. Remove seemingly unused constant. Could not find a usage for this. And since MethodHandles is not available on android, I simply removed it for the time being. See #4004. Revert to old behavior, when indy disabled. Just like in 8db25932200fb3ed7160066b184e6db2e29f1921 Do not init JITCompiler if it is not used. See #4004. Use jruby's ClassValue. Instead of java.lang.ClassValue which ist not available on android. See #4004. Move idTest to where it is actually needed. This of course destroys any gains of caching the idTest, but gets rid of MethodHandle et al in RubyModule which is important to run on android. See #4004. Remove unused Method. Since this will not run on android is not being used currently anyway, just comment it out for now. See #4004. Revert "relax seek-channel type requirement from FileChannel to SeekableByteChannel" This reverts commit dc48f34. No SeekableByteChannel on android. See #4004. Work around missing java.nio libs on android. See #4004. Replace ClassValue with internal implementation. Because java.lang.ClassValue is not available on android. See #4004. Add minimal MethodHandle implementation. java.lang.invoke.Method* classes are not available on android. See #4004. Add code generation for android. See #4004. Move nullToNil out of the way. To not have MethodHandles in Ruby.java as this is not available on android. See #4004. Guard against missing JITCompiler. Necessry as JITCompiler will not get instantiated on android. See #4004. Remove unused methods getIdTest + newIdTest Move Android logic to the Ruboto project Removed extra white space Remove the maven repository for dexmaker

headius

Overall it's a good approach. We need to address the loss of cached MethodHandle for non-Android, and there's a few questions to be addressed.

headius · 2017-05-07T01:36:16Z

core/src/main/java/org/jruby/RubyFile.java

-        return ctime(context);
-    }
-
-    public static final FileTime getBirthtimeWithNIO(String pathString) {


Mild concern here about removing a public method, but I don't know how long it has been around.

headius · 2017-05-07T01:39:27Z

core/src/main/java/org/jruby/RubyModule.java

@@ -299,18 +300,6 @@ public void addIncludingHierarchy(IncludedModule hierarchy) {
        }
    }

-    public MethodHandle getIdTest() {


Same here but it was probably wrong of me to expose it in the first place.

headius · 2017-05-07T01:41:58Z

core/src/main/java/org/jruby/ir/targets/Bootstrap.java

-import org.jruby.runtime.DynamicScope;
-import org.jruby.runtime.Frame;
-import org.jruby.runtime.ThreadContext;
+import org.jruby.runtime.*;


We prefer not to do * except in the most extreme cases. This isn't one of them.

headius · 2017-05-07T01:42:29Z

core/src/main/java/org/jruby/ir/targets/Bootstrap.java

@@ -39,6 +34,7 @@
 import org.objectweb.asm.Opcodes;

 import java.lang.invoke.*;
+import java.lang.invoke.CallSite;


This * is my fault. We should just expand it.

headius · 2017-05-07T01:43:34Z

core/src/main/java/org/jruby/ir/targets/Bootstrap.java

@@ -657,7 +653,10 @@ public static IRubyObject ivarGet(VariableSite site, IRubyObject self) throws Th
        VariableAccessor accessor = realClass.getVariableAccessorForRead(site.name());

        // produce nil if the variable has not been initialize
-        MethodHandle nullToNil = self.getRuntime().getNullToNilHandle();


The purpose of getNullToNilHandle was to have a single place to cache the handle and reuse it everywhere. This is going to recreate handles every time. I agree we need a better place than on Ruby, but we still need these one-time handles.

headius · 2017-05-07T01:43:48Z

core/src/main/java/org/jruby/ir/targets/Bootstrap.java

@@ -657,7 +653,10 @@ public static IRubyObject ivarGet(VariableSite site, IRubyObject self) throws Th
        VariableAccessor accessor = realClass.getVariableAccessorForRead(site.name());

        // produce nil if the variable has not been initialize
-        MethodHandle nullToNil = self.getRuntime().getNullToNilHandle();
+        IRubyObject nilObject = self.getRuntime().getNil();
+        MethodHandle nullToNil = lookup().findStatic(Helpers.class, "nullToNil", methodType(IRubyObject.class, IRubyObject.class, IRubyObject.class));


Same issue here with excessive handle creation.

headius · 2017-05-07T01:44:18Z

core/src/main/java/org/jruby/ir/targets/ConstantLookupSite.java

+        MethodHandles.Lookup lookup = MethodHandles.lookup();
+        MethodHandle testModuleMatch = null;
+        try {
+            testModuleMatch = lookup.findStatic(Bootstrap.class, "testModuleMatch", MethodType.methodType(boolean.class, ThreadContext.class, IRubyObject.class, int.class));


Same issue here with excessive module creation. I'm sure we can find a way/place to save these handles without polluting things so much that Android doesn't work.

headius · 2017-05-07T01:45:47Z

core/src/main/java/org/jruby/runtime/BlockBody.java

@@ -50,7 +50,7 @@
 public abstract class BlockBody {

    protected final Signature signature;
-    protected volatile MethodHandle testBlockBody;


This is not used but might be later. It is also to avoid recreating the same handle over and over again. It could be made Object and casted elsewhere to avoid the hard reference of MethodHandle.

headius · 2017-05-07T01:46:55Z

core/src/main/java/org/jruby/util/invoke/MethodHandle.java

+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+
+public class MethodHandle {


This part is pretty nice, and I almost wonder if we should pull it out into its own library, for others to use.

headius · 2017-05-07T01:47:58Z

core/src/main/java/org/jruby/util/io/ChannelFD.java

@@ -131,7 +130,7 @@ private void initChannelTypes() {
        else chRead = null;
        if (ch instanceof WritableByteChannel) chWrite = (WritableByteChannel)ch;
        else chWrite = null;
-        if (ch instanceof SeekableByteChannel) chSeek = (SeekableByteChannel)ch;


What's the reason for this? SeekableByteChannel is not a new class, is it? I suppose on current Java the only SeekableByteChannel is FileChannel, but I prefer to use the more generic interface if possible.

The reason for this is that SeekableByteChannel was added to Android 7.0 API level 24 ref.:

https://developer.android.com/reference/java/nio/channels/SeekableByteChannel.html

FileChannel has been in Android since the initial release.

I agree that using the interface is the better option, and we can do a patch in Ruboto if necessary, although better if we don't have to 😄 .

Ugh. I'd really rather not use FileChannel everywhere, even though I don't know of any SeekableByteChannel offhand (even jnr-enxio channels don't implement the interface). And changing chSeek to be a FileChannel makes it essentially just a duplicate of chFile.

I hate to make more work here but this is a pretty icky change to have to make. I wish Google's incorporation of Java 7 APIs weren't so arbitrary.

I'll do a patch in Ruboto, then.

OK, reinstated the use of SeekableByteChannel and put a copy of the ChannelFD source in Ruboto.

donv · 2017-05-07T07:02:30Z

Thanks for the review! I can fix the import expansions.

Do you prefer I force-push those changes, or add a new commit?

headius · 2017-05-09T16:34:32Z

@donv I don't care if you do it as a separate commit or not. The original set of commits was long and has already been squashed, so we don't really have much history left in this PR anyway.

donv · 2017-05-09T19:40:22Z

OK, I fixed the '*' imports. Which of the other comments need to be done before merging?

kares · 2017-05-09T21:16:37Z

core/src/main/java/org/jruby/util/invoke/MethodType.java

@@ -0,0 +1,53 @@
+package org.jruby.util.invoke;
+
+import org.jruby.javasupport.ext.JavaLangReflect;


donv · 2017-05-10T18:39:30Z

OK, reinstated SeekableByteChannel. What more needs to be done? I am on it in a flash! ⚡️

headius · 2017-05-10T21:15:20Z

@donv The places where we removed MethodHandle from e.g. Ruby, RubyModule, etc, should be reinstated, but signatures changed to something Android supports (maybe just Object) and the creation of the handles indirected through e.g. OptoFactory. We need to still be able to cache those handles rather than recreating them every time. nullToNil, for example.

headius · 2017-05-10T21:23:47Z

@donv It occurs to me we could use generics to fix those methods; when called with a MethodHandle factory, they'd return MethodHandle, so type-safety on non-Android would still be restored. I may try to prototype that this evening.

donv · 2017-05-11T06:48:30Z

Cool! Looking forward to it 😄

headius · 2017-06-13T17:38:54Z

Ok...obviously I didn't get back to it before my trip.

I'll review what we have now.

headius · 2017-06-13T18:50:15Z

@donv Sorry for the long delay but I made my changes. Since there were only two places where we had removed handle caches, I just restored them as all Object signatures. Unless used (by JIT+indy) they shouldn't get called and shouldn't trigger load of Java 7+ indy classes.

Review my changes please! I think this could go into 9.2. @enebo?

headius · 2017-06-13T19:46:06Z

So I realized that the travis build has not been green. It appears that it's failing because the JIT is still running and using invokedynamic for globals and constants, but indy defaults to false which disables the SwitchPoint-based invalidators for constants and globals.

I'll see what I can figure out.

headius · 2017-06-13T19:48:23Z

Problem stems from this change, which provided a non-indy way to create a constant invalidator: 1f31168

We'll need to modify the JIT to have non-indy global and const logic.

This is not a straight reversion of the caching commit below, since there were modifications to how constants bound after that change. Note that we still need to restore constant caching for non-indy paths, or otherwise make it easier to say that Java 7+ JRuby will use indy for some things without compile.invokedynamic, but disable those features on platforms that don't support indy. Revert "Always use invokeynamic to lookup and cache constants." This reverts commit e03c4f1.

This is a completely unoptimized path, and can't be shipped, but it should continue to allow things to work on Android without breaking the non-indy JIT for JRuby proper.

headius · 2017-06-13T21:30:16Z

I have pushed a few more commits that get things further along:

Constants and globals now have (uncached, slow) non-indy logic in the JIT. This allows them to build right with indy disabled and still use indy when it is turned on. More on this below
Another small fix to the nullToNil cached filter.

We can't really ship this for JRuby proper without providing some non-indy caching for constants and globals, since that would represent a fairly bad regression. At the very least we need a better way to pick and choose features based on profile: for example, on desktop Java 7 we may use a few indy features even if full indy is not turned on, but on Android we must avoid all indy features but may be able to use some other 7 features.

Rake still seems to be having trouble starting up, now due to an NPE in the IR interpreter. @enebo

headius · 2017-06-13T23:04:49Z

Tests appear to be going green now, yay. We'll see how it looks and then see about optimizing these non-indy compiler changes.

headius · 2017-06-14T13:05:01Z

I'm confused by the MRI failures. A handful of tests that attempt to run in a subprocess seem to be timing out, in this and other builds (see also the spec:ruby:fast timeout).

I'll poke at things today and try to figure it out.

donv · 2017-06-16T07:10:04Z

@headius This is looking great! Seems we are getting close!

headius · 2017-06-19T18:01:54Z

@donv Whatever's causing those two failures seems to be affecting all branches, so we may actually be good to go with the branch as-is.

donv · 2017-06-19T18:37:50Z

Woohoo!

donv · 2017-06-22T18:24:37Z

@headius Good news! I used the jruby-jars from this branch and upgraded one of our production apps today. And it is now operational! Sure, it is a simple app with very few in-house users, but it works well 😄

I encountered some problems I had to work around, but I'd like to get this PR merged before I report those.

You mentioned we may be ready to merge this PR? Doable? Yes?

donv · 2017-07-09T08:05:14Z

@headius Can we merge this issue now? I have a problem with subclassing a Java class on Android, but it would be good to work off JRuby master with this PR merged.

donv · 2017-08-31T10:17:15Z

Hi @headius !

Master has moved on a bit. I can try to resolve the conflicts unless you want to do it.

donv force-pushed the ruboto_9k branch from 9181a50 to 1f31168 Compare May 5, 2017 20:51

donv changed the title ~~Return ObjectIdentityInvalidator if indy disabled.~~ Allow JRuby 9K to run on Ruboto May 5, 2017

donv requested a review from headius May 5, 2017 21:28

headius requested changes May 7, 2017

View reviewed changes

Expand '*' imports as per JRuby coding convention

9629da0

kares reviewed May 9, 2017

View reviewed changes

UweKubosch added 2 commits May 10, 2017 10:34

Reinstate the use of SeekableByteChannel. Patch in Ruboto instead.

2834c3c

Remove unused import

4707f66

headius added 3 commits June 13, 2017 13:36

Reinstate nullToNil handle cache but move Indy logic out of Ruby.

e82715c

Reinstate idTest cache in RubyModule without tying to indy.

eb64b9a

Just remove block body indy experiment; won't live here likely.

786eef5

headius added this to the JRuby 9.2.0.0 milestone Jun 13, 2017

headius approved these changes Jun 13, 2017

View reviewed changes

headius added 3 commits June 13, 2017 16:04

Fix signatures and casting for nullToNil.

8b8add6

Provide a non-indy global var path again.

67820f8

This is a completely unoptimized path, and can't be shipped, but it should continue to allow things to work on Android without breaking the non-indy JIT for JRuby proper.

Always initialize JITCompiler but move indy stuff out of top cls.

41b1cda

Merge branch 'master' into ruboto_9k

ded9644

Add non-indy caching of GlobalVariable accessors.

4d1b0c1

enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018

headius modified the milestones: JRuby 9.2.1.0, JRuby 9.2.2.0 Nov 1, 2018

enebo modified the milestones: JRuby 9.2.2.0, JRuby 9.2.3.0, JRuby 9.2.4.0, JRuby 9.2.5.0 Nov 8, 2018

enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018

headius removed this from the JRuby 9.2.6.0 milestone Dec 18, 2018

		@@ -0,0 +1,53 @@
		package org.jruby.util.invoke;

		import org.jruby.javasupport.ext.JavaLangReflect;

Allow JRuby 9K to run on Ruboto #4589

Are you sure you want to change the base?

Allow JRuby 9K to run on Ruboto #4589

Conversation

donv commented May 5, 2017

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

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

donv May 10, 2017 • edited Loading

Choose a reason for hiding this comment

donv commented May 7, 2017

headius commented May 9, 2017

donv commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donv commented May 10, 2017

headius commented May 10, 2017

headius commented May 10, 2017

donv commented May 11, 2017

headius commented Jun 13, 2017

headius commented Jun 13, 2017

headius commented Jun 13, 2017

headius commented Jun 13, 2017

headius commented Jun 13, 2017

headius commented Jun 13, 2017

headius commented Jun 14, 2017

donv commented Jun 16, 2017

headius commented Jun 19, 2017

donv commented Jun 19, 2017

donv commented Jun 22, 2017

donv commented Jul 9, 2017

donv commented Aug 31, 2017

donv May 10, 2017 •

edited

Loading