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

Thu Jun 13 15:40:40 UTC 2019

On 2019-06-13 16:33, Maurizio Cimadamore wrote:
> Looks really good.
> 
> The code is really clean, I have nothing to object as to the choices
> you've made.

Thanks!

> 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 :-)

Yep - we generate a class per arity, but the actual values of the 
strides can differ per VarHandle instance.

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

Okay.

> 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`

Thanks for the suggestion! I knew there must have been a utility laying 
around for that :)

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

Thanks for catching this.

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

:)

Updated webrev: 
https://cr.openjdk.java.net/~jvernee/panama/webrevs/8225172/webrev.04/

The sizeOf(Class) call was also implicitly doing a carrier type check, 
so I'm now doing that explicitly in 
MemoryAccessVarHandles::dereferenceVarHandle and 
LayoutPathImpl::dereferenceVarHandle instead, and added a test case for 
this as well.

Cheers,
Jorn

> 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