[foreign-memaccess] RFR 8237648: Add support for var handle adaptation

Fri Jan 24 21:13:49 UTC 2020

On 24/01/2020 17:44, Paul Sandoz wrote:
> MethodHandles
> --
>
> 5207      * If the filter returns {@code void}, the target must accept all coordinates
> 5208      * not passed to the filter.
> ...
> 5235     public static VarHandle collectCoordinates(VarHandle target, int pos, MethodHandle filter) {
>
> I am still uncertain of this use-case, and if we should allow it.
>
>
>> On Jan 23, 2020, at 6:07 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>> Here's a new webrev which addresses previous comments, and also adds the logic to check whether provided method handle filters are 'throwy' or not.
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8237648_v2/
>>
>> Regarding MethodHandles::throwException, I did few experiments and I concluded that it wasn't worth special casing. The MH returned by this method is, essentially, MethodHandlesImpl::throwException - which is declared to throw Throwable. So, the current logic will detect that, and issue an error.
>>
> Ok, the current logic looks good.  I think under the circumstances it is reasonable to reject rather than automatically wrap (the developer would have to do that).

We could drop this use case.

We could similarly drop filterCoordinates and just retain 
collectArgument (which, if provided with unary filter behaves the same, 
right) ?

Also, should I move these combinators into the MemoryHandles class (in 
the incubating module) or do we prefer to leave them here in preparation 
for possible upstream to jdk/jdk?

Maurizio

>
> Paul.
>
>> Refining the logic looking at what actual exception type has been used to create the throwy MH is very fragile - that's because of the way MethodHandles::throwException, which basically just obtains a MH for MethodHandlesImpl::throwException, and _adapts_ that so that the parameter type matches that of the exception type selected by the user. This means that it will be extremely hard to distinguish between system-provided adaptation and user-defined ones.
>>
>> If we really really want to close this hole (but I think honestly it feels like a corner case), we could rework MethodHandleImpl so that we have two named functions - throwException and throwRuntimeException, with the right 'throws clauses' so that the logic I wrote will pick that up naturally. But, as I said, I don't feel like this is important enough to solve - what we have will err on the conservative side, which seems like a sound move.
>>
>> Cheers
>> Maurizio
>>
>> On 23/01/2020 10:42, Maurizio Cimadamore wrote:
>>> Good point.
>>>
>>> I'll take it for a spin and report back.
>>>
>>> Maurizio
>>>
>>> On 23/01/2020 01:49, Paul Sandoz wrote:
>>>> It’s also possible to create a throwing method handle with:
>>>>
>>>>     public static MethodHandle throwException(Class<?> returnType, Class<? extends Throwable> exType) {
>>>>
>>>> But I think that is still amenable to the scanning approach you suggest (look for the named function throwException and the method type arg to see if a checked exception).
>>>>
>>>> It would be nice if any method handle could be structurally introspected with a public API.  Instead we can traverse the LFs.
>>>>
>>>> Paul.
>>>>
>>>>> On Jan 22, 2020, at 3:59 PM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>>
>>>>> Thinking more about this... it wouldn't be prohibitive to check for the validity of the method handles passed to the adapter methods.
>>>>>
>>>>> After all... the only method handles which can throw checked exceptions are the direct ones. And for a direct MH, we can crack it and see if it throws.
>>>>>
>>>>> All the bound method handles cannot introduce _new_ checked exceptions - but can of course propagate some of the checked exceptions thrown by some of the underlying target MHs.
>>>>>
>>>>> But, given a bound MH, we can always look a its bound arguments and do a recursive scan, to find any MH that is both 'direct' and 'throwy'.
>>>>>
>>>>> So, it seems there is a path to apply a validation and make sure no (checked) exceptions are thrown?
>>>>>
>>>>> Maurizio
>>>>>
>>>>> On 22/01/2020 21:16, Maurizio Cimadamore wrote:
>>>>>> Thanks for the feedback Paul,
>>>>>> what do you think on the topic of exceptions which Remi (correctly) brought up? VarHandle don't throw, but if you combine them with throwy MHs, they can start to throw...
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>> On 22/01/2020 20:02, Paul Sandoz wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> That’s really nice how this slotted into the existing infrastructure.  Quite a powerful feature.  I wondered when VHs were being developed whether we could and should do something like this but a strong enough use-case did not present itself at that time.
>>>>>>>
>>>>>>> There is this commented out class in VarHandles called GuardMethodGenerator that generates the VarHandleGuards source code.  I try to keep that in sync with the actual code.  I can update it afterwards.
>>>>>>>
>>>>>>>
>>>>>>> MethodHandles
>>>>>>> —
>>>>>>>
>>>>>>> 4957     public static VarHandle filterValue(VarHandle target, MethodHandle unbox, MethodHandle box) {
>>>>>>>
>>>>>>> The terms boxing and unboxing may be misleading in terms of the corresponding MHs functionality.  I think they are dual mapping functions from domains S -> T and T -> S, as stated clearly in the documentation.  Perhaps s/unbox/filterToTarget and s/filterFromTarget ?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 5104     /**
>>>>>>> 5105      * Provides a var handle which adapts the coordinate values of the target var handle, by re-arranging them
>>>>>>> 5106      * so that the new coordinates match the provided ones.     *
>>>>>>>
>>>>>>> Rogue “*” at end of line.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 5225     public static VarHandle collectCoordinates(VarHandle target, int pos, MethodHandle filter) {
>>>>>>>
>>>>>>> When the filter returns void it drop coordinates.  Is that useful? Are there cases were some coordinates would be redundant?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 5244     private static List<Class<?>> coordinates(VarHandle handle) {
>>>>>>> 5245         return handle.accessModeType(VarHandle.AccessMode.GET).parameterList();
>>>>>>> 5246     }
>>>>>>>
>>>>>>>
>>>>>>> Usages of this method can be replaced with handle.coordinateTypes()
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I suspect to complete the set of combinators a useful addition is an expanding or spreading combinator e.g. taking a single coordinate type of say Index(x, y) and substituting with x and y.  However, it's a little tricky with the existing MH functionality and I believe it requires filtering the composite coordinate into an array of its component values and then spreading the array elements to the separate target arguments. I am not confident that optimizes well, esp. for coordinates of different types.
>>>>>>>
>>>>>>> Paul.
>>>>>>>
>>>>>>>
>>>>>>>> On Jan 22, 2020, at 6:35 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>> this patch add support for some VarHandle adapters:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8237648/
>>>>>>>>
>>>>>>>> More specifically, the following adaptations are supported:
>>>>>>>>
>>>>>>>> * filterValue
>>>>>>>> * insertCoordinates
>>>>>>>> * filterCoordinates
>>>>>>>> * collectCoordinates
>>>>>>>> * permuteCoordinates
>>>>>>>>
>>>>>>>> This fixes an expressiveness mismatch between VarHandle and MethodHandles, as one could adapt MethodHandles, but cannot adapt VarHandles (the only way to do that is to map a VarHandle to a MethodHandle - using an AccessMode - and then use the MethodHandle).
>>>>>>>>
>>>>>>>> To achieve this, only minimal surgery to j.l.invoke classes was required: the basic idea is to introduce a new class of VarHandle, namely IndirectVarHandle, which is merely an aggregate of a bunch of method handles, one per supported access modes. This means that we have now the ability to take apart an existing var handle into its MH components, adapt such components and repackage them into a brand new VarHandle.
>>>>>>>>
>>>>>>>> To save unnecessary bytecode spinning, the component MethodHandles of an indirect VarHandle are only created on-demand - that is, if you only ever use the GET access mode - the machinery will not create the method handles for all the access modes which could be in principle be supported by the new var handle.
>>>>>>>>
>>>>>>>> Since adapting a VarHandle might introduce changes in some of the coordinate types, I've disabled the VarHandleGuards fastpath for indirect handles, as the fast path does an invokeBasic and it skips any kind of type adaptation. While invokeBasic will detect e.g. a long vs. int mismatch, it does not detect (by design) a String vs. Runnable mismatch, so it is preferrable for indirect handles to always have an asType() adaptation. If the client is doing things "by the books" and using exact types in the VarHandle call, the asType() will do nothing and performances will not be negatively affected.
>>>>>>>>
>>>>>>>> I also needed to tweak lambda form generation for var handles a bit, to obtain the direct var handle associated with an indirect handle before calling the linker method (otherwise we might end up the underlying VarHandle routine with the wrong VarHandle type).
>>>>>>>>
>>>>>>>> I think overall, this implementation strategy proved to be flexible and relatively efficient - we can easily add as many adapters as we want, since we can build on the already rich MethodHandle combinator API.
>>>>>>>>
>>>>>>>> Cheers
>>>>>>>> Maurizio
>>>>>>>>
>>>>>>>>