Thanks Paul, I'll do a pass and fix the issues you found.
Some comments inline:
On 06/12/2019 21:04, Paul Sandoz wrote:
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?
Uhm - I might have messed up something... but I think the fields should
be removed from the superclass - it's only the concrete subclasses using
them, and they are being used inside synchronized blocks for their
corresponding classes - so I think they are meant to be disjoint. I
think I added them by accident in Unmapper.
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?
I'm on the fence on that one. Yes for completeness it should be there...
but doesn't seem all that useful either. I can see that, pulling on that
string, you end up with an optional size (since sometimes the size isn't
there) but that means all clients pay the price for the few layouts that
use unspecified sequences. I was hoping to keep them under the rug to be
honest.
MemoryAddress.java
—
Should MemoryAddress implement Comparable?
I don't think so - as they don't form an order - two addresses are
comparable only if their segments are the same.
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)
Thanks for catching these
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)
Gotcha
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?
Not really, I agree this should just use Objects::hashCode
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.
Thanks
Maurizio
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
