Array load/store and length with invokedynamic#18232
Conversation
|
Looks great. Thanks, the current Array get/set is really horrible, although in my tests the getLength is not so bad. So i think its best to do what you have, eliminate usage of this reflection class alltogether. Its performance is inconsistent and cannot be trusted! Can you also update the docs to explain a bit more (https://github.com/uschindler/elasticsearch/blob/array_invoke_dynamic/modules/lang-painless/src/main/java/org/elasticsearch/painless/Def.java#L50) ? We may want to also test assigning different array types to Thanks again! |
|
OK, I added a test class ArrayTests which also uses a for loop to calculate the sum of the aritmetic series. I also updated docs to add array loads and stores to indy section in Def. |
|
FYI: Reflective Array.get/Array.set are slow like hell - some pointers and issues at the OpenJDK bug tracker are here: http://stackoverflow.com/questions/30306160/performance-of-java-lang-reflect-array, https://bugs.openjdk.java.net/browse/JDK-8051447 People say like 20 times slower :-) |
c6ff54f to
2e613f4
Compare
|
Thanks @uschindler for taking care of this. |
|
FYI: There is now an issue for the |
After @rmuir's improvements for using invokedynamic (#18201) with dynamic method calls and field load/stores, this extends the whole thing to array loads and stores.
It also optimizes access to
array.lengthby using a specialization for every array type. This works around a missing method in Java's MethodHandles class (e.g.,MethodHandles.arrayLengthGetter(Class<?> arrayType)).I did not do performance checks for now, because @rmuir knows better how to do that in ES - I just coded it :-)