I mostly looked at the API and implementation and not the tests.
s/offset/add or plus ? add ‘l’ to the offset of this memory address the result
of which is the offset of the returned memory address.
If we ever have controlled operator overloading that’s how I would like to
express it :-)
Paul.
VarHandles.java
—
40 static ClassValue<Map<Integer, MethodHandle>> addressFactories = new
ClassValue<Map<Integer, MethodHandle>>() {
41 @Override
42 protected Map<Integer, MethodHandle> computeValue(Class<?> type) {
43 return new ConcurrentHashMap<>();
44 }
45 };
s/addressFactories/ADDRESS_FACTORIES/
Perhaps expose as ConcurrentMap to express concurrency requirements.
May be useful to pre-size the map.
JavaNioAccess.java
—
52 /**
53 * Constructs an heap ByteBuffer with given backing array, offset,
capacity and segment.
54 */
55 ByteBuffer newHeapByteBuffer(byte[] hb, int offset, int capacity,
MemorySegmentProxy segment);
56
Formatting issue.
FileChannelImpl.java
—
867 private static abstract class Unmapper
868 implements Runnable, UnmapperProxy
869 {
870 // may be required to close file
871 private static final NativeDispatcher nd = new
FileDispatcherImpl();
872
873 // keep track of mapped buffer usage
874 static volatile int count;
875 static volatile long totalSize;
876 static volatile long totalCapacity;
These new field declarations are also declared on the concrete subtypes. Did
you intend to remove the declarations from the latter?
VarHandles
—
I see you are relying on the fallback to lambda forms when linking the var
handle, otherwise the explicit guard methods just explode :-)
Clever generation of VH code on demand punching in live constants, while
pre-cooking accessors for “base" access.
AddressVarHandleGenerator.java
--
355 private int returnInsn(Class<?> type) {
356 switch (LambdaForm.BasicType.basicType(type)) {
357 case I_TYPE: return Opcodes.IRETURN;
358 case J_TYPE: return Opcodes.LRETURN;
359 case F_TYPE: return Opcodes.FRETURN;
360 case D_TYPE: return Opcodes.DRETURN;
361 case L_TYPE: return Opcodes.ARETURN;
362 case V_TYPE: return RETURN;
363 default:
364 throw new InternalError("unknown return type: " + type);
365 }
366 }
Replace with expression form? (Same for following method too) I believe "JEP
361: Switch Expressions (Standard)” is integrated.
GroupLayout.java
—
80 OptionalLong sizeof(List<MemoryLayout> elems) {
81 long size = 0;
82 for (MemoryLayout elem : elems) {
83 if (AbstractLayout.optSize(elem).isPresent()) {
84 size = sizeOp.applyAsLong(size, elem.bitSize());
85 } else {
86 return OptionalLong.empty();
87 }
88 }
89 return OptionalLong.of(size);
90 }
FWIW you can do this:
OptionalLong sizeof(List<MemoryLayout> elems) {
return elems.stream().filter(e ->
AbstractLayout.optSize(e).isPresent()).mapToLong(MemoryLayout::bitSize)
.reduce(sizeOp);
}
It may be question why there is no way to query if a MemoryLayout has a size or
not. Does it require a hasSize method?
MemoryAddress.java
—
Should MemoryAddress implement Comparable?
MemoryHandles.java
—
* As an example, consider the memory layout expressed by a {@link
SequenceLayout} instance constructed as follows:
* <blockquote><pre>{@code
SequenceLayout seq = MemoryLayout.ofSequence(5,
MemoryLayout.ofStruct(
MemoryLayout.ofPaddingBits(32),
MemoryLayout.ofValueBits(32).withName("value")
));
* }</pre></blockquote>
MemoryLayout.ofValueBits requires a byte order argument e.g.:
MemoryLayout.ofValueBits(32, ByteOrder.BIG_ENDIAN)
* <blockquote><pre>{@code
VarHandle handle = MemoryHandles.varHandle(int.class); //(MemoryAddress) -> int
handle = MemoryHandles.withOffset(handle, 4); //(MemoryAddress) -> int
handle = MemoryHandles.withStride(handle, 8); //(MemoryAddress, long) -> int
* }</pre></blockquote>
MemoryHandles.varHandle requires a byte order argument e.g.:
VarHandle handle = MemoryHandles.varHandle(int.class, ByteOrder.BIG_ENDIAN);
//(MemoryAddress) -> int
(See also SequenceLayout)
105 public static VarHandle varHandle(Class<?> carrier, ByteOrder
byteOrder) {
You may need to specify what access modes are supported, as is the case for
MethodHandles.byteBufferViewVarHandle, and also specify how comparison is
performed for float/double with atomic update access modes i.e. copy and paste
appropriate text. (Same applies to MemoryLayout.varHandle)
SequenceLayout.java
—
118 @Override
119 public int hashCode() {
120 return super.hashCode() ^ elemCount.hashCode() ^
elementLayout.hashCode();
121 }
I commonly resort to using Objects.hashCode. Don’t have a strong preference
here. I guess you might be concerned about efficient?
package-info.java
—
30 * <pre>{@code
31 static final VarHandle intHandle = MemoryHandles.varHandle(int.class);
32
33 try (MemorySegment segment = MemorySegment.allocateNative(10 * 4)) {
34 MemoryAddress base = segment.baseAddress();
35 for (long i = 0 ; i < 10 ; i++) {
36 intHandle.set(base.offset(i * 4), (int)i);
37 }
38 }
39 * }</pre>
MemoryHandles.varHandle requires a byte order argument.
(I wish we could compile code snippets in JavaDoc to surface errors.)
MemoryAddressImpl
—
48 public MemoryAddressImpl(MemorySegmentImpl segment) {
49 this(segment, 0);
50 }
IDE reporting this constructor is never used.
MemorySegmentImpl.java
—
61 final static long NONCE = new Random().nextLong();
If a better quality NONCE is required do ‘new SplittableRandom()..nextLong();’
186 private final boolean isSet(int mask) {
187 return (this.mask & mask) != 0;
188 }
Unnecessary final modifier.
Utils.java
—
143 MemoryScope scope = new MemoryScope(null, () ->
unmapperProxy.unmap());
Replace with method ref.
> On Dec 5, 2019, at 1:04 PM, Maurizio Cimadamore
> <maurizio.cimadam...@oracle.com> 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
>
