> On Dec 10, 2019, at 4:51 AM, Maurizio Cimadamore
> <maurizio.cimadam...@oracle.com> wrote:
>
> And, another, small iteration
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5
>
> Delta compared to previous version (v4):
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5_delta
>
> Javadoc updated in place:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
>
>
>
> Summary of changes:
>
> * Peter pointed out that the logic for checking overflow in
> MemoryScope::acquire was bogus, as once we had an overflow, the segment was
> rendered useless (as the count stayed in negative overflowed state) -
> separately, Paul pointed out that it would be better to roll in our atomic
> loops, and just use a VarHandle instead of atomic integer - I've rewritten
> MemoryScope quite a bit, to implement these changes; I think the result is
> much cleaner than before. Note that, in the new code, the boolean flag is
> gone, and we just use the int count for everything (thanks to Jorn for the
> nice observation).
MemoryScope changes look good.
In j.u.concurrent we use ExceptionInInitializerError in the static blocks when
there is an error looking up the VH,
FWIW close can also fail because it has already been closed but the exception
message does not distinguish between the two states (active or already closed).
Paul.
>
> * Peter also pointer out that the isAlive() check is not thread-safe; I now
> have named the liveness-check-related methods explicitly (and added some
> javadoc), so that it's clear when they can be called safely
>
> * removed call to checkAlive from MemorySegment::acquire -
> MemoryScope;:acquire will check that anyway
>
> * fixed rough javadoc comment on MemoryLayout::withName
>
> * dropped non-overlapping restriction on MemoryAddress::copy (and fixed test
> accordingly)
>
> Maurizio
>
>
> On 09/12/2019 19:23, Maurizio Cimadamore 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
>>>>>
