8264288: Performance issue with MethodHandle.asCollector #3306
This patch speeds up MethodHandle.asCollector handles where the array type is not Object, as well as speeding up all collectors where the arity is greater than 10.
The old code is creating a collector handle by combining a set of hard coded methods for collecting arguments into an Object, up to a maximum of ten elements, and then copying this intermediate array into a final array.
In principle, it shouldn't matter how slow (or fast) this handle is, because it should be replaced by existing bytecode intrinsification which does the right thing. But, through investigation it turns out that the intrinsification is only being applied in a very limited amount of cases: Object with max 10 elements only, only for the intermediate collector handles. Every other collector shape incurs overhead because it essentially ends up calling the ineffecient fallback handle.
Rather than sticking on a band aid (I tried this, but it turned out to be very tricky to untangle the existing code), the new code replaces the existing implementation with a collector handle implemented using a LambdaForm, which removes the need for intrinsification, and also greatly reduces code-complexity of the implementation. (I plan to expose this collector using a public API in the future as well, so users don't have to go through MHs::identity to make a collector).
The old code was also using a special lambda form transform for collecting arguments into an array. I believe this was done to take advantage of the existing-but-limited bytecode intrinsification, at least for Object with less than 10 elements.
The new code just uses the existing collect arguments transform with the newly added collector handle as filter, and this works just as well for the existing case, but as a bonus is also much simpler, since no separate transform is needed. Using the collect arguments transform should also improve sharing.
As a result of these changes a lot of code was unused and has been removed in this patch.
Testing: tier 1-3, benchmarking using TypedAsCollector (part of the patch), as well as another variant of the benchmark that used a declared static identity method instead of MHs::identity (not included). Before/after comparison of MethodHandleAs* benchmarks (no differences there).
Here are some numbers from the added benchmark:
(as you can see, just the Object with arity less than 10 case is fast here)
rose00 left a comment
So many red deletion lines; I'm looking at a beautiful sunset!
Thanks very much for cleaning this out. See a few
@JornVernee This change now passes all automated pre-integration checks.
After integration, the commit message for the final commit will be:
At the time when this comment was updated there had been 55 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
PaulSandoz left a comment
That's an elegant solution.
At first i thought it might unduly perturb lambda form generation and caching. but you slotted a different lambda form implementation underneath the varargs implementation.
I've addressed review comments, plus some other things:
As a result, the lambda form is now fully intrinsified (no more calls in the generated bytecode) e.g.:
Addressed latest review comments:
I also had to switch back to the un-cached version for creating the array element setter, since the cached version casts the array to be a specific type, and if the lambda form is shared, this won't work (e.g. casting an Object to String, depending on the order of creating the collectors). Adding caching there is left as a followup.
I also did some benchmarks where I introduced profile pollution manually (by using collectors of different reference types a bunch in the static initializer), but this didn't affect the results, so I think we're indeed safe there.
@JornVernee Since your change was applied there have been 59 commits pushed to the
Your commit was automatically rebased without conflicts.
Pushed as commit b7baca7.