ReadyTalk / avian

[INACTIVE] Avian is a lightweight virtual machine and class library designed to provide a useful subset of Java's features, suitable for building self-contained applications.
https://readytalk.github.io/avian/
Other
1.22k stars 172 forks source link

Add support for "invokevirtual" and "invokeinterface". #539

Closed chrisr3 closed 7 years ago

chrisr3 commented 7 years ago

Trying to use a function reference on Avian (compiled against OpenJDK):

List<String> list = asList(args);
int totalChars = list.stream().mapToInt(String::length).sum();

resulted in this error:

todo: implement per http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3.5

Following the link revealed that Avian lacked support for invokevirtual, which is now trivially(?) fixed. For an encore, I then implemented invokeinterface as well.

chrisr3 commented 7 years ago

This looks like an enhancement to #530. The required test-case is as above.

dicej commented 7 years ago

Thanks for doing this, Chris.

Would you mind adding tests for both the invokevirtual and invokeinterface cases to your PR? We already have a number of lambda tests in test/InvokeDynamic.java, so that would be a good place to add them.

chrisr3 commented 7 years ago

I have found this test case for invokeinterface:

BiFunction<CharSequence, Integer, Character> f = CharSequence::charAt;
String data = "0123456789";
for (int i = 0; i < data.length(); ++i) {
    System.out.println(f.apply(data, i));
}

Unfortunately, I've now realised that my implementation here is broken. I am using this document as a reference for a correct implementation, but am wondering how to derive the count byte that it mentions. My initial thought was that it would be 1 + method.parameterFootprint. However, Avian doesn't seem to account for each long or double parameter contributing 2 to the final footprint value, as described in the Notes. Am I interpreting parameterFootprint correctly here, please?

Edit: Actually, I've just done a clean rebuild and it appears that count should be method.parameterFootprint, with long and double already being accounted for. But Avian is still crashing.

chrisr3 commented 7 years ago

All of my new InvokeDynamic test cases involving function references that use native types are currently failing. However, these cases execute correctly with other JVMs. For example:

BiFunction<CharSequence, Integer, Character> f = CharSequence::charAt;

In this case, Avian seems to be tripping over the auto-unboxing of Integer to int, resulting in

java/lang/StringIndexOutOfBoundsException: String index out of range: -1857339888
  at java/lang/String.charAt (line 658)
  at Lambda-8.apply (unknown line)
  at InvokeDynamic.test (line 147)
  at InvokeDynamic.main (line 76)
chrisr3 commented 7 years ago

The problem I'm seeing here with native types is more subtle than I had originally suspected. E.g.

public class TestInvokeInterfaceLongBad {

  private interface BiFunction<T, U, R> {
    R apply(T t, U u);
  }

  private interface GetLong {
    long get(long l);
  }

  private static class LongHolder implements GetLong {
    @Override
    public long get(long l) {
        return l;
    }
  }

  public static void main(String[] args) {
      BiFunction<GetLong, Long, Long> f = GetLong::get;
      System.out.println("Long: " + f.apply(new LongHolder(), 20L));
  }
}

This fails because Avian cannot find interface TestInvokeInterfaceLongBad$GetLong on its Lambda-0 class. However, if I rewrite the GetLong interface to use Long instead of native long then Avian searches for TestInvokeInterfaceLongGood$GetLong on TestInvokeInterfaceLongGood$LongHolder instead. And this works.

dicej commented 7 years ago

Hi Chris,

Thanks for working on this, and sorry I haven't been very responsive. I'll investigate the issues you found this weekend when I have time.

And yes, parameterFootprint should account for double and long parameters taking two slots.

dicej commented 7 years ago

This fixes all the test cases you added: https://github.com/ReadyTalk/avian/commit/ecafdf40baf2f3cde4835620e9af4380b1081fe7

Thanks for finding those cases and taking the first steps towards fixing them.