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

[Maurizio Cimadamore]

Sure, I'll tweak that.

Maurizio

On 23/01/2020 14:37, Remi Forax wrote:
> Fine for me,
> just in MethodHandles.noCheckedExceptions:
>    Class<?>[] exceptionTypes = null;
>    switch (info.getReferenceKind()) {
> is a code smell now that we have a switch expression.
>
> Can be written as:
>    Class<?>[] exceptionTypes = switch (info.getReferenceKind()) {
>      case ...
>      default -> throw WhateverException()
>   }
> or better list the constants (getfield, getstatic, putfield, putstatic) instead of using "default", so if a new constant is added it will fail to compile.
>
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "Maurizio Cimadamore" <maurizio.cimadamore at oracle.com>
>> À: "Paul Sandoz" <paul.sandoz at oracle.com>
>> Cc: "panama-dev at openjdk.java.net'" <panama-dev at openjdk.java.net>
>> Envoyé: Jeudi 23 Janvier 2020 15:07:00
>> Objet: Re: [foreign-memaccess] RFR 8237648: Add support for var handle adaptation
>> 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.
>>
>> 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
>>>>>>>>