+1
Paul.
> On Dec 9, 2019, at 11:23 AM, Maurizio Cimadamore
> <maurizio.cimadam...@oracle.com> wrote:
>
> Another iteration:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/
>
> Delta of the changes since last version (v3) here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4_delta/
>
> The javadoc has been updated inline here:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
>
>
> Summary of changes:
>
> * Improved javadoc regarding alignment and access modes in MemoryHandles
> * related to the above, the memory access varhandle now checks for low-level
> VM alignment to for access other than get/set (as happens for regular byte
> buffer view var handle)
> * fixed MemoryHandles, JavaLangInvokeAccess, VarHandles, MethodHandleImpl to
> speak about alignmentMask, rather than alignment (as per Roger comments)
> * added some more javadoc to internal classes MemoryAddressProxy,
> VarHandleMemoryAddressBase, JavaNioAccess.java and JavaLangInvokeAccess.java
> (as per Roger comments)
> * fixed mising {code} block around alignment param A in
> MemoryLayout::bitAlignment/bytesAlignment (as per offline comments from
> Daniel)
> * added positive test to TestMemoryCopy
>
> Cheers
> Maurizio
>
> On 08/12/2019 01:44, Maurizio Cimadamore wrote:
>> Another update:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3/
>>
>> And a delta of the changes since last version (v2) here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3_delta/
>>
>> The javadoc has been updated inline here:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
>>
>>
>> Summary of changes:
>>
>> * fixed some (cosmetic) issues in FileChannelmpl following offline
>> discussion with Alan
>> * changed tests group definition, so that now jdk_foreign is part of
>> tier1_part3
>> * updated javadoc in various places to fix code samples (as per Paul
>> comments)
>> * updated javadoc in MemoryHandles to talk about access modes (as per Paul
>> comments) - I added a new section on top, and then referred to in from all
>> relevant places
>> * some changes to use Objects.hash (as per Paul comments), and some minor
>> refactor in the AddreddGenerator (to use switch expression)
>> * tightened javadoc for MemoryAddress::copy - the method now is specified to
>> throw IAE if the range of source/dest addresses overlap - I've fixed the
>> impl and added a test (as per Raffaello comments)
>> * tightened byte buffer VarHandle view - if the view is created from a byte
>> buffer obtained from a segment (!!!) we should do a segment check - added
>> tests
>>
>>
>> And here's a list of the pending API-related issues. Since these are IMHO
>> minor issues, I suggest we defer them to a followup minor cleanup, so that
>> we can move ahead with finalization of the CSR with the current API. Here's
>> the list:
>>
>> * Should MemoryAddress implement Comparable? I think the answer is "probably
>> not" - an address doesn't have a 'length' so you can't really do a range
>> comparison (maybe memory segments though?). For now, users can project to
>> byte buffer and take it from there (since ByteBuffer <: Comparable)
>> * should we have a way to ask a Layout if its size is specified ? (Paul)
>> * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much
>> distance between these two semantically different operations (Paul suggested
>> using 'add' - which I'm not enthusiastic about because it's not adding two
>> addresses - it's adding a long to an address...)
>> * MemorySegment::isAccessible() -- as the A* word is overloaded, some other
>> name should be found?
>> * MemorySegment::acquire() -- replace with MemorySegment::fork?
>>
>> Thanks
>> Maurizio
>>
>> On 06/12/2019 10:43, Maurizio Cimadamore wrote:
>>> Hi,
>>> here's an updated version of the patch:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2/
>>>
>>> And a delta of the changes since last version here:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2_delta/
>>>
>>> The javadoc has been updated inline here:
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
>>>
>>>
>>> Summary of changes:
>>>
>>> * fixed spurious protected methods in AbstractLayout and subclasses which
>>> leaked into API
>>> * removed library_call.cpp changes, as these are being tracked separately
>>> by Vlad
>>> * compacted ILOAD code in AddressVarHandleGenerator
>>> * replaced uses of ++i/--i with i + 1/i - 1 in MemoryScope
>>>
>>> I have made no changes to the *name* of the methods in the API. In fact, I
>>> suggest we keep a list of the names we'd like to revisit, and we address
>>> them all at once at the end of the review (once we're happy with the
>>> contents). Here's a list of the current open naming issues:
>>>
>>> * MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much
>>> distance between these two semantically different operations
>>> * MemorySegment::isAccessible() -- as the A* word is overloaded, some other
>>> name should be found?
>>> * MemorySegment::acquire() -- replace with MemorySegment::fork?
>>>
>>> Cheers
>>> Maurizio
>>>
>>>
>>> On 05/12/2019 21:04, Maurizio Cimadamore wrote:
>>>> Hi,
>>>> as part of the effort to upstream the changes related to JEP 370 (foreign
>>>> memory access API) [1], I'd like to ask for a code review for the
>>>> corresponding core-libs and hotspot changes:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8234049/
>>>>
>>>> A javadoc for the memory access API is also available here:
>>>>
>>>> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
>>>>
>>>>
>>>> Note: the patch passes tier1, tier2 and tier3 testing (**)
>>>>
>>>>
>>>> Here is a brief summary of the changes in java.base and hotspot (the
>>>> remaining new files are implementation classes and tests for the new API):
>>>>
>>>> * ciField.cpp - this one is to trust final fields in the foreign memory
>>>> access implementation (otherwise VM doesn't trust memory segment bounds)
>>>>
>>>> * Modules.gmk - these changes are needed to require that the incubating
>>>> module is loaded by the boot loader (otherwise the above changes are
>>>> useless)
>>>>
>>>> * library_call.cpp - this one is a JIT compiler change to treat
>>>> Thread.currentThread() as a well-known constant - which helps a lot in the
>>>> confinement checks (thanks Vlad!)
>>>>
>>>> * various Buffer-related changes; these changes are needed because the
>>>> memory access API allows a memory segment to be projected into a byte
>>>> buffer, for interop reasons. As such, we need to insert a liveness check
>>>> in the various get/put methods. Previously we had an implementation
>>>> strategy where a BB was 'decorated' by a subclass called ScopedBuffer -
>>>> but doing so required some changes to the BB API (e.g. making certain
>>>> methods non-final, so that we could decorate them). Here I use an approach
>>>> (which I have discussed with Alan) which doesn't require any public API
>>>> changes, but needs to add a 'segment' field in Buffer - and then have
>>>> constructors which keep track of this extra parameter.
>>>>
>>>> * FileChannel changes - these changes are required so that we can reuse
>>>> the Unmapper class from the MemorySegment implementation, to
>>>> deterministically deallocate a mapped memory segment. This should be a
>>>> 'straight' refactoring, no change in behavior should occur here. Please
>>>> double check.
>>>>
>>>> * VarHandles - this class now provides a factory to create memory access
>>>> VarHandle - this is a bit tricky, since VarHandle cannot really be
>>>> implemented outside java.base (e.g. VarForm is not public). So we do the
>>>> usual trick where we define a bunch of proxy interfaces (see
>>>> jdk/internal/access/foreign) have the classes in java.base refer to these
>>>> - and then have the implementation classes of the memory access API
>>>> implement these interfaces.
>>>>
>>>> * JavaNIOAccess, JavaLangInvokeAccess - because of the above, we need to
>>>> provide access to otherwise hidden functionalities - e.g. creating a new
>>>> scoped buffer, or retrieving the properties of a memory access handle
>>>> (e.g. offset, stride etc.), so that we can implement the memory access API
>>>> in its own separate module
>>>>
>>>> * GensrcVarHandles.gmk - these changes are needed to enable the generation
>>>> of the new memory address var handle implementations; there's an helper
>>>> class per carrier (e.g. VarHandleMemoryAddressAsBytes, ...). At runtime,
>>>> when a memory access var handle is needed, we dynamically spin a new VH
>>>> implementation which makes use of the right carrier. We need to spin
>>>> because the VH can have a variable number of access coordinates (e.g.
>>>> depending on the dimensions of the array to be accessed). But, under the
>>>> hood, all the generated implementation will be using the same helper class.
>>>>
>>>> * tests - we've tried to add fairly robust tests, often checking all
>>>> possible permutations of carriers/dimensions etc. Because of that, the
>>>> tests might not be the easiest to look at, but they have proven to be
>>>> pretty effective at shaking out issues.
>>>>
>>>> I think that covers the main aspects of the implementation and where it
>>>> differs from vanilla JDK.
>>>>
>>>> P.S.
>>>>
>>>> In the CSR review [2], Joe raised a fair point - which is MemoryAddress
>>>> has both:
>>>>
>>>> offset(long) --> move address of given offset
>>>> offset() --> return the offset of this address in its owning segment
>>>>
>>>> And this was considered suboptimal, given both methods use the same name
>>>> but do something quite different (one is an accessor, another is a
>>>> 'wither'). one obvious option is to rename the first to 'withOffset'. But
>>>> I think that would lead to verbose code (since that is a very common
>>>> operation). Other options are to:
>>>>
>>>> * rename offset(long) to move(long), advance(long), or something else
>>>> * drop offset() - but then add an overload of MemorySegment::asSlice which
>>>> takes an address instead of a plain long offset
>>>>
>>>> I'll leave the choice to the reviewers :-)
>>>>
>>>>
>>>>
>>>> Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad, Stuart,
>>>> Roger, Joe and the Panama team for the feedback provided so far, which
>>>> helped to get the API in the shape it is today.
>>>>
>>>> Cheers
>>>> Maurizio
>>>>
>>>> (**) There is one failure, for "java/util/TimeZone/Bug6329116.java" - but
>>>> that is unrelated to this patch, and it's a known failing test.
>>>>
>>>> [1] - https://openjdk.java.net/jeps/370
>>>> [2] - https://bugs.openjdk.java.net/browse/JDK-8234050
>>>>
