[foreign-abi] RFR: 8238192: Reimplement MemoryAddress memory access handles on top of var handle combinators

[Maurizio Cimadamore]

On Thu, 13 Feb 2020 14:01:32 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This is a continuation of the RFR here:
>> https://mail.openjdk.java.net/pipermail/panama-dev/2020-January/007366.html
> 
> It's unfortunate that AddressVarHandleGenerator still needs to be aware of the MemoryAddressProxy as a carrier. It would be nice if this wasn't the case, and it only knew about `long`, and we could handle the adaptation in places where we call JLI.memoryAddressViewVarHandle. The downside with the current setup is that AddressVarHandleGenerator silently changes MemoryAddressProxy to long, and expects code that is elsewhere (Utils::fixUpVarHandle) to apply the adaptation. These Seem prone to going out of sync...
> 
> For changing where the adaptation happens, the only problematic places seem to be MemoryHandles::withOffset and MemoryHandles::withStride. Since they use JLI.memoryAddressCarrier to find the carrier type. I also notice they re-wrap the direct VarHandle target, so any adaptation is discarded... I think we should require a direct VarHandle as a base for these methods, i.e. modify MethodHandleImpl::checkMemAccessHandle to throw if `handle.isDirect()` returns false, since we don't seem to be able to apply to re-apply the adaptations any ways (at least now). That would also mean that VarHandles with a MemoryAddress carrier would be disallowed for withOffset and withStride, and maybe that is  okay.
> 
> But, that should also allow us to remove knowledge about MemoryAddressProxy as a carrier from AddressVarHandleGenerator and instead handle the adaptation in MemoryHandles::varHandle and LayoutPath::dereferenceHandle instead, by requesting a VarHandle with 'long' as a carrier, and then applying the adaptation. Then all the adaptation happens in one place.

> It's unfortunate that AddressVarHandleGenerator still needs to be aware of the MemoryAddressProxy as a carrier. It would be nice if this wasn't the case, and it only knew about `long`, and we could handle the adaptation in places where we call JLI.memoryAddressViewVarHandle. The downside with the current setup is that AddressVarHandleGenerator silently changes MemoryAddressProxy to long, and expects code that is elsewhere (Utils::fixUpVarHandle) to apply the adaptation. These Seem prone to going out of sync...


Well, I tried doing it the other way and ran into a roadblock with the MemoryHandles methods - as yu note below.


> 
> For changing where the adaptation happens, the only problematic places seem to be MemoryHandles::withOffset and MemoryHandles::withStride. Since they use JLI.memoryAddressCarrier to find the carrier type. I also notice they re-wrap the direct VarHandle target, so any adaptation is discarded... I think we should require a direct VarHandle as a base for these methods, i.e. modify MethodHandleImpl::checkMemAccessHandle to throw if `handle.isDirect()` returns false, since we don't seem to be able to apply to re-apply the adaptations any ways (at least now). That would also mean that VarHandles with a MemoryAddress carrier would be disallowed for withOffset and withStride, and maybe that is okay.

Except none of the memory access handles are really direct - since we have to adapt them all to fixup the MemoryAddressProxy mismatch. Also, not super sure about the restriction on MemoryAddress adaptations...

> 
> But, that should also allow use to remove knowledge about MemoryAddressProxy as a carrier from AddressVarHandleGenerator and instead handle the adaptation in MemoryHandles::varHandle and LayoutPath::dereferenceHandle instead, by requesting a VarHandle with 'long' as a carrier, and then applying the adaptation. Then all the adaptation happens in one place.

While I see where you want to go (I wanted to go there too), I don't see a straightforward path to get there. I think that, as we discussed at some point, the real issue here is that the adapters in MemoryHandles should really be dropped in favor of "real" VH adapters - but we can't go down there (at the moment) because to do those kind of adaptations you would need to use MemoryAddress::addOffset, which is slow (because of outstanding VM/C2 issues).

So, while I get the discomfort with the approach in this patch, I don't think we can do much better if we carry the MemoryHandles adapters around. And, as you note, this patch has issues if clients start mixing regular VH adapters with MemoryHandles adapters. I don't know how to solve that, given that we are using adaptation internally as a result of JDK-8237349.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/19