Hi Remi, > > while working and trying the new JDK9 MethodHandles features like > > MethodHandles#countedLoop, I recognized a API inconsistency problem > with it. > > > > We also found this while implementing the "Painless" scripting language for > > Elasticsearch (see https://goo.gl/DbOzjC)! Painless is a very limited > > scripting language with a very limited subset of classes to access and close > > integration into Elasticsearch (similar to Lucene's Expressions Module). > > Both compile script code to anonymous classes and Elasticsearch uses > > invokedynamic for the untyped stuff. It is much faster than Groovy or > > Nashorn, because it compiles most of the code completely static with > known > > types (like Lucene expressions), but also supports a Groovy-like "def" type. > > The latter one uses invokedynamic, everything else is exactly the same > speed > > as native Java code. > > I think you can do something similar in Groovy with the annotation > @TypeChecl and @StaticCompilation, see [1].
The reason why we not use Groovy is security-related. It is impossible to limit access to only a very limited subset of Java classes and methods with Groovy. Because of many security issues Groovy is no longer an option. But that is unrelated to this issue, of course :-) The script snippets in Painless or Lucene expressions are scripts that are executed multiple times on millions of search results during collecting fulltext search hits. E.g. when you want to customize the score. For that to work it is important that the compiled script is using primitive types for score and so on. > > During implementing array getters and setters for untyped stuff with > > invokedynamic we recognized the following problem (Java 8 code, see > > https://github.com/elastic/elasticsearch/pull/18232): > > > > There is MethodHandles.arrayElementGetter and > > MethodHandles.arrayElementSetter, so we can have full speed and full > dynamic > > flexibility for array loads and stores - the speed is great, same as native > > array access! But there is one factory method missing in Java 8: > > MethodHandles.arrayLengthGetter (or similar name). As "length" is not a > > field on the array class, it is impossible to create a "simple" MethodHandle > > as field access to "array.class#length" to access it (it must invoke the > > "arraylength" bytecode, there is no field). I think the "length" field is > > just syntactic sugar of javac compiler. > > > > I know that Nashorn used to use java.lang.reflect.Array#getLength (not > sure > > if this is still the case), but this makes usage inconsistent. You have to > > explicitly create a MethodHandle using the reflective array class to use it: > > And it is not strongly typed (it accepts Object). A real MethodHandle would > > be created for a specific type of array class and would return its length > > without runtime type checks. I know that at least Array.getLength() is > > optimized by Hotspot (in contrast to the other get/set methods, which are > > like 20 times slower than a method access, so > > MethodHandles.arrayElementGetter/Setter is very useful), but I would > really > > like to suggest to fix this! > > > > With Java 9 this gets a bit worse: There is no "easy way" with the > > MethodHanldes class to generate a MethodHandles.countedLoop() on all > > elements of an array: > > > > public static MethodHandle countedLoop(MethodHandle iterations, > MethodHandle > > init, MethodHandle body) [new in Java 9] > > > > With a full-featured API one could write: > > > > Class<?> type = arraytype, e.g. long[].class or String[].class > > MethodHandle body = some code that uses > > MethodHandles.arrayElementGetter(type) for further processing > > MethodHandles.countedLoop(MethodHandles.arrayLengthGetter(type), > > MethodHandles.constant(int.class, 0), body); > > > > As you see, for the first parameter (count), you would need to use the > > reflective part in java.lang.reflect.Array if the method is still missing in > > Java 9. This is not bad here, because it is not called all the time, but for > > our scripting language, the reflective class was slower. > > > > We implemented our own version of "arrayLengthGetter": > > > > public class ArrayLengthHelper { > > private ArrayLengthHelper() {} > > > > private static final Lookup PRIV_LOOKUP = MethodHandles.lookup(); > > private static final Map<Class<?>,MethodHandle> > ARRAY_TYPE_MH_MAPPING = > > Collections.unmodifiableMap( > > Stream.of(boolean[].class, byte[].class, short[].class, int[].class, > > long[].class, char[].class, float[].class, double[].class, > > Object[].class) > > .collect(Collectors.toMap(Function.identity(), type -> { > > try { > > return PRIV_LOOKUP.findStatic(PRIV_LOOKUP.lookupClass(), > > "getArrayLength", MethodType.methodType(int.class, type)); > > } catch (ReflectiveOperationException e) { > > throw new AssertionError(e); > > } > > })) > > ); > > private static final MethodHandle OBJECT_ARRAY_MH = > > ARRAY_TYPE_MH_MAPPING.get(Object[].class); > > > > static int getArrayLength(boolean[] array) { return array.length; } > > static int getArrayLength(byte[] array) { return array.length; } > > static int getArrayLength(short[] array) { return array.length; } > > static int getArrayLength(int[] array) { return array.length; } > > static int getArrayLength(long[] array) { return array.length; } > > static int getArrayLength(char[] array) { return array.length; } > > static int getArrayLength(float[] array) { return array.length; } > > static int getArrayLength(double[] array) { return array.length; } > > static int getArrayLength(Object[] array) { return array.length; } > > > > public static MethodHandle arrayLengthGetter(Class<?> arrayType) { > > if (!arrayType.isArray()) { > > throw new IllegalArgumentException("type must be an array"); > > } > > return (ARRAY_TYPE_MH_MAPPING.containsKey(arrayType)) ? > > ARRAY_TYPE_MH_MAPPING.get(arrayType) : > > > OBJECT_ARRAY_MH.asType(OBJECT_ARRAY_MH.type().changeParameterTy > pe(0, > > arrayType)); > > } > > } > > > > Interestingly I later found out that > MethodHandles.arrayElementGetter/Setter > > uses the same "trick" behind the scenes! See > MethodHandleImpl.ArrayAccessor: > > http://goo.gl/94f6OB > > > > I would suggest to add the missing Method to MethodHandles class and > > implement it together with the getters and setters in ArrayAccessor, similar > > to our example code (it is just a few lines more). In addition this one > > could then also use the extra intrinsic improvement that our class cannot > > use: makeIntrinsic(getAccessor(Object[].class, false), > > Intrinsic.ARRAY_LOAD); > > [with another intrinsic added: Intrinsic.ARRAY_LENGTH for the arraylength > > bytecode] > > > > What would be the process to propose such a change? Bug/Issue/JEP? > > I could quickly create a patch, but fixing the Intrinsic/LambdaForm stuff is > > way too complicated for me, so I would suggest to take this as an > > inspiration how to do it > > > > Uwe > > as you said, Array.getLength is already optimized by the JIT, it's because the > length of an array is at the same offset for any kind of array in Hotspot, so > unlike array access that depends on the kind of array, getting the length of > an > array is the same code for any kind of array. So you can safely, performance > wise, create a method handle on Array.getLength, that's why there is no > arrayLengthGetter in MethodHandles. That might be fine, but it is not user friendly. You have to spend a lot of time to get this running. If the API would be consistent the above countedLoop would be a no-brainer to implement. I don’t care if the MethodHandles.arrayLengthGetter is implemented with java.lang.reflect.Array or the proposed code in MethodHandlesImpl.ArrayAccessor. It should just be abstracted and an implementation detail. Because the array length is something very special, so there should be an "official and easy to understand way" to get a MethodHandle for it, that is consistent with the other API. MethodHandles has accessor to array elements but not to length - that seems wrong - really wrong, sorry! If you think that java.lang.reflect.Array is fine because of the intrinsic, then just implement MethodHandles.arrayLengthGetter like: private static final MH_ARRAY_LENGTH = publicLookup().findStatic(Array.class, "getLength", methodType(int.class, Object.class); // add trycatch public static MethodHandle arrayLengthGetter(Class<?> arrayType) { if (!arrayType.isArray()) { throw new IllegalArgumentException("arrayType must be an array"); } return MH_ARRAY_LENGTH.asType(MH_ARRAY_LENGTH.type().changeParameterType(0, arrayType)); } It should just be there! The change is simple (both cases), so why not add it? What do others think? Uwe
