I cleaned up and ran benchmarks: this is currently a script that runs on all of geonames and looks like this: (Math.log(Math.abs(input.doc['population'].value)) + input.doc['elevation'].value * input.doc['latitude'].value)/_score

I altered the way we access document fields and values to show perf for different dynamic language features:

.value shows the hack from #18169 working: we remove the hack here and have the same performance.
.getValue(): no-arg dynamic method
.field.0: two field loads, one that was especially slow before (the .0)
.field.get(0): field load + dynamic method with parameter

The static version (with casts) is ~ 0.517333 s, so there isn't much overhead. I benchmark with the scripts inline so these times include compilation and JIT costs.

Also I want to defer trying to "improve" the signature we create here. Currently we often know the real types of parameters, e.g. get(0), we know the one parameter is an int, we don't need to box it, but we explicitly do a lot of conversions anyway. That is because the current code is setup to work that way. It is silly to keep it at Object long-term but I want to fix the super slow calls first.

We can later refactor to improve that (e.g. all tests pass if i do this: rmuir@e029b9e but thats really hacky).

This is phenomenal - nice work!

This is phenomenal - nice work!

++

Ho Robert, I have seen you made the suggested changes already. Thanks! To me this looks fine - I only reviewed the CallSite part. Looks great.

I would also do the other optimizations in separate issues, like Array.getLength() (which may be slower than a real access to the length field in bytecode) to some static&private getters (see your TODO). The problem is that getting the array length is only a property in Java syntax, but behind the scenes all is a separate bytecode for array load/store/length: https://en.wikipedia.org/wiki/Java_bytecode_instruction_listings

P.S.: Thanks for the fruitful discussions this weekend! The whole MethodHandle stuff is so great. I was happy to help and add suggestions. I will review the remaining code a bit later and provide some feedback if there are questions open.

We are now running simple script search performance tests in the nightly classic benchmarks: https://benchmarks.elastic.co/index.html#search_qps_scripts

It shows already a big speedup (I will add annots!) for painless dynamic from the last few improvements.

Hi Robert,
I created the MethodHandle code for the array.length bytecode: https://gist.github.com/uschindler/916930741f245204e418c56025a8b58a

This code has a static method where you can pass an primitive or x-extends-Object array and you get a MethodHandle to retun its length. This should be faster than the reflective Array.getLength (because it is statically compiled and we have a separate method for every primitive type). Maybe we shold benchmark it.

Basically it holds a static map array->type to methodhandle (dynamically build) for primitives and Object[], and if the actual type is not found there (e.g, String[]), it adds a necessary cast to Object[].

The code has test as main() method inline.

Thanks Uwe, I will fold in your ArrayUtil class instead of calling the slow Array.getLength.

LGTM. Thanks for doing this!

@uschindler for the array length, i want to defer that to another issue. I don't want us having possibility of slowness, but Array.getLength is an intrinsic method, it may not really be slow. We can benchmark it to see.

@rmuir that's a great improvement. :) I think that besides looking at the median, it's also worth looking at the percentile distribution (esp. higher percentiles like 99%, 99.9%) to get a better overall impression. Just out of curiosity: Did you have a chance to have a look at these too?

@danielmitterdorfer The nightly benchmark is just to explain what the differences mean at a high level, it is not really what I used to develop the changes. That benchmark is extremely noisy and only outputs the median.

This isn't a micro-optimization, I didn't tune anything with any benchmark. A branch in the code + exact methodhandle call is night and day vs looking up a methodhandle from our whitelist and adapting it, and the situation is well studied and understood. Its just important to use correct datastructures for the problem (e.g. polymorphic cache) so that we don't call lookups over and over.

@rmuir Oh, I didn't expect that you'd use the benchmark to tune anything (and I also know that they're quite noisy. I think this is partly system-inherent but we should (a) also ensure that we eliminate as much noise as possible on the platform where we run nightlies and (b) adopt approaches that help us to deal with noise (both topics are on my todo list)). Thanks for your explanation.

The nightlies could use big improvement. I think traditionally effort has been on indexing performance and not query performance. All the queries are noisy and nothing is setup for e.g. a developer to iterate/explore with (means not reindexing every time you run the benchmark, and being reasonably fast).

For scripts, we also only test a fictional search script to at least have something. But for example, we don't even have a fictional update script yet, forget about having any variety in the types of scripts we want to benchmark or anything like that.

@uschindler for the array length, i want to defer that to another issue. I don't want us having possibility of slowness, but Array.getLength is an intrinsic method, it may not really be slow. We can benchmark it to see.

Yes, there should be a separate issue abbout the arrays and array-like structures. The arrayLoad and arrayStore methods in Def can be removed completely and similarly handled by invokeDynamic. This is especially important because the reflective Array.get/Array.set are slow like hell (see http://stackoverflow.com/questions/30306160/performance-of-java-lang-reflect-array, https://bugs.openjdk.java.net/browse/JDK-8051447).

If we use invokeDyanmic here, too, the bootstrap method (maybe a different bootstrap method - not sure) can select the right MethodHandles correctly based on types. Partly it is already implemented for the map gets, this can be done in a similar way for the list get/sets and using MethodHandles.arrayElementGetter/Setter. This should bring a huge improvement for array accesses!

+1 as a first step. LGTM

Thanks @uschindler for reviewing.

@rmuir thanks for your feedback. I have created elastic/rally#96 and elastic/rally#97. Feel free to create more tickets if you find more shortcomings or just approach me. I really appreciate your input.

@danielmitterdorfer thanks for opening those issues! FYI I hope it didn't come across as a criticism of rally, just our current state, lots of room for improvement.

@rmuir You're welcome. Your feedback did not come as criticism at all, quite the contrary. I know that we have a lot of room for improvement and I also have lots of ideas on how to improve the situation but I really value your feedback.