[foreign-memaccess] RFR 8225172: Add way to create deref VarHandles without using the Layout API

Thu Jun 13 14:33:03 UTC 2019

Looks really good.

The code is really clean, I have nothing to object as to the choices 
you've made.

I wonder if we can do some further consolidation in the VarHandles code 
- that is, we pass the 'dims' to the generator - if we passed the stride 
array directly (after all, 'dims' == 'strides.length'), we could then 
use CP patching for that too in the accessor, which will simplify the 
code quite a bit... but I also realize that this would send the caching 
logic off to the drain, so... nvm :-)

Also, I would check for negative strides in the VH constructor and throw 
eagerly if that's not valid, rather than wait until VarHandles.

Another nit is that I find the 'sizeof(Class)' routine repeated in 
multiple places:

* LayoutPathImpl
* VarHandles
* MemoryAccessVarHandles

This should be consolidated somehow. One option is also to use 
`sun.invoke.util.Wrapper.findPrimitive(carrier).bitWidth() / 8`

Finally, there's a typo in the javadoc for MemoryAccessVarHandles: 'it's 
accesses' vs. 'its accesses'.

Overall, it seems like we have a nice set of Lego bricks here :-)

Maurizio

On 13/06/2019 14:02, Jorn Vernee wrote:
> Hi,
>
> I've made a new version that works like the proposed combinator-like API.
>
> Webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.03/
>
> This adds a new class called MemoryAccessVarHandles to 
> java.lang.invoke which has these combinators:
>
>   VarHandle dereferenceVarHandle(Class<?> carrier)
>   VarHandle offsetHandle(VarHandle handle, long offset)
>   VarHandle elementHandle(VarHandle handle, long scale)
>   VarHandle alignAccess(VarHandle handle, long align)
>   VarHandle byteOrder(VarHandle handle, ByteOrder order)
>
> I put this class in j.l.i to get easy access to 
> VarHandleMemoryAddressBase, which is used to get the needed 
> information out of a VarHandle to use in a combinator call.
>
> In order to get the carrier type and strides array out of an existing 
> VarHandle I added to accessor methods to VarHandleMemoryAddressBase, 
> which are spun dynamically. Adding them as fields could work as well, 
> but this was my attempt to keep the footprint of the VarHandle 
> instance smaller.
>
> Cheers,
> Jorn
>
> On 2019-06-04 02:15, Jorn Vernee wrote:
>> Maurizio Cimadamore schreef op 2019-06-04 01:41:
>>> Hi Jorn,
>>> I too feel some of the tension you are talking about, but I'm quite
>>> opposed to a builder API such as the one you described.
>>
>> Yeah - this was more of a suggestion to get the conversation going, in
>> hindsight.
>>
>>> Rather, what I'd be interested to explore, is whether we could built
>>> complex dereference expressions out  of simpler ones, using an
>>> approach similar to MethodHandle combinators. Let's assume that each
>>> memory access VH has an offset, denoted as O. Then it seems to me that
>>> there are only a bunch of moves we want to allow:
>>>
>>> * obtain a memory address dereference for a given carrier type - this
>>> is the entry point VH, whose O = zero
>>>
>>> * starting from a given memory address VH with offset O, obtain a new
>>> VH' whose O' =  O + C (where C is an argument to the combinator)
>>>
>>> * starting from a given memory address VH with offset O, obtain a new
>>> one, called VH' whose offset O' = (S * N) + O (where S is an argument
>>> to the combinator - the scale - and N is a dynamic argument to VH' -
>>> the array index)
>>>
>>> So, let's say I want to access the i32 in the layout below:
>>>
>>> [5 : [ x32 i32 ] ]
>>>
>>> What I'd do would be something like:
>>>
>>> VarHandle handle = MemoryAddresses.dereferenceHandle(int.class); 
>>> //(MA) -> I
>>> handle = MemoryAddresses.offsetHandle(handle, 4); //(MA) -> I
>>> handle = MemoryAddresses.elementHandle(handle, 8); //(MA, J) -> I
>>>
>>>
>>> I think this is expressive enough to do what our API currently allows,
>>> and I kind of like that it gets there in a more primitive way (and
>>> very similar to how MH are constructed bottom up).
>>
>> Yes, this seems much better than my idea :)
>>
>> One thing I like about it is that we immediately get a VarHandle for
>> the most simple case, rather than having to do several API calls to
>> get there.
>>
>>> Whether I'd like to completely give up Layout/LayoutPath for something
>>> like this I'm not sure though. The ability of constructing a full
>>> memory layout and then obtaining a var handle from one of its path
>>> seems pretty good. I've written examples with this API where going
>>> down the Layout route immediately spotted latent alignment issues that
>>> were present in the Unsafe code that it replaced. So, while we can get
>>> there in a more primitive way, I'm not 100% sure of whether that means
>>> we should get rid of layouts too. At the very least, layouts are
>>> useful whenever there's some kind of size/alignment/offset computation
>>> to be done, which, with layout, can be expressed at a slightly higher
>>> level.
>>
>> Agree here as well. I just entertained the thought of removing
>> Layouts, but also remembered that this is something that people
>> actaully asked for w.r.t. direct ByteBuffers. So I think it's good to
>> keep. But at the same time I'd like to see the Layout API being
>> decoupled a bit more from the deref VarHandle API. Having a separate
>> API layer in between seems like a good way to assure that, while also
>> offering users the additional option to user their own layout APIs.
>>
>> Jorn
>>
>>> Maurizio
>>>
>>>
>>>
>>>
>>> On 03/06/2019 17:46, Jorn Vernee wrote:
>>>> Forgot to 'hg add' one file. See updated webrev: 
>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.02/
>>>>
>>>> Thanks,
>>>> Jorn
>>>>
>>>> Jorn Vernee schreef op 2019-06-03 18:38:
>>>>> Hi,
>>>>>
>>>>> I've implemented an alternative way of creating memory access
>>>>> VarHandles, which bypasses the Layout API. One reason for having such
>>>>> an alternative API is that it offers users the opportunity to use
>>>>> their own (pre-existing) layout APIs when creating VarHandles. This
>>>>> mainly consists of a builder API called MemoryAccessVarHandleBuilder,
>>>>> which can be used similarly to LayoutPath to construct a VarHandle.
>>>>>
>>>>> As a side effect I've removed the dependency on Layout/LayoutPath 
>>>>> from
>>>>> VarHandles::makeMemoryAddressViewHandle. I think this decoupling 
>>>>> makes
>>>>> this API point more suited to what's possible to do with memory 
>>>>> access
>>>>> VarHandles.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225172
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.01/
>>>>>
>>>>> Added tests are pretty minimal since the resulting VarHandle is
>>>>> already tested by the existing tests using the Layout API.
>>>>>
>>>>> This also opens up the possibility of removing the Layout API
>>>>> altogether if we wanted. Another TODO is adding a way to disable
>>>>> alignment checking upon access (which would need changes to the
>>>>> VarHandle impl classes I think).
>>>>>
>>>>> Thanks,
>>>>> Jorn