Re: RFR: 8286715: Generalize MemorySegment::ofBuffer [v2]

2022-05-16 Thread Maurizio Cimadamore
> This patch makes MemorySegment::ofBuffer more general, by allowing clients to 
> pass *any* `Buffer` instance, not just `ByteBuffer`.
> This allows us to match expressiveness of JNI API, where JNI clients can 
> obtain the address of any direct buffer instance, using the 
> `GetDirectBufferAddress` function.
> 
> We thought about also providing a more general way to view a segment as a 
> buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct 
> buffers can only created form `ByteBuffer`.
> So, to create a direct `IntBuffer`, clients have to first create a direct 
> `ByteBuffer` then to view that buffer as an `IntBuffer`.
> 
> In other words, `IntBuffer` and friends are not first-class citizens in the 
> `Buffer` API. As such it would not be possible to map many memory segments 
> into an `IntBuffer`; in fact, the only segment we could safely map into an 
> `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it 
> doesn't seem worth adding a lot of API surface (in terms of additional 
> overloads) for such a corner case.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Update 
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
  
  Co-authored-by: Jorn Vernee 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8701/files
  - new: https://git.openjdk.java.net/jdk/pull/8701/files/02494e2f..b6629787

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8701=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8701=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8701/head:pull/8701

PR: https://git.openjdk.java.net/jdk/pull/8701


Re: RFR: 8286715: Generalize MemorySegment::ofBuffer

2022-05-16 Thread Jorn Vernee
On Fri, 13 May 2022 12:33:10 GMT, Maurizio Cimadamore  
wrote:

> This patch makes MemorySegment::ofBuffer more general, by allowing clients to 
> pass *any* `Buffer` instance, not just `ByteBuffer`.
> This allows us to match expressiveness of JNI API, where JNI clients can 
> obtain the address of any direct buffer instance, using the 
> `GetDirectBufferAddress` function.
> 
> We thought about also providing a more general way to view a segment as a 
> buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct 
> buffers can only created form `ByteBuffer`.
> So, to create a direct `IntBuffer`, clients have to first create a direct 
> `ByteBuffer` then to view that buffer as an `IntBuffer`.
> 
> In other words, `IntBuffer` and friends are not first-class citizens in the 
> `Buffer` API. As such it would not be possible to map many memory segments 
> into an `IntBuffer`; in fact, the only segment we could safely map into an 
> `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it 
> doesn't seem worth adding a lot of API surface (in terms of additional 
> overloads) for such a corner case.

Marked as reviewed by jvernee (Reviewer).

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java 
line 572:

> 570: return new NativeMemorySegmentImpl(bbAddress + (pos << 
> scaleFactor), size << scaleFactor, modes, bufferSession);
> 571: } else {
> 572: // we can ignore scale factor here, a mapped buffer is 
> always a byte buffer, so scaleFactor == 1.

Suggestion:

// we can ignore scale factor here, a mapped buffer is always a 
byte buffer, so scaleFactor == 0.

-

PR: https://git.openjdk.java.net/jdk/pull/8701


Re: RFR: 8286715: Generalize MemorySegment::ofBuffer

2022-05-13 Thread Maurizio Cimadamore
On Fri, 13 May 2022 12:33:10 GMT, Maurizio Cimadamore  
wrote:

> This patch makes MemorySegment::ofBuffer more general, by allowing clients to 
> pass *any* `Buffer` instance, not just `ByteBuffer`.
> This allows us to match expressiveness of JNI API, where JNI clients can 
> obtain the address of any direct buffer instance, using the 
> `GetDirectBufferAddress` function.
> 
> We thought about also providing a more general way to view a segment as a 
> buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct 
> buffers can only created form `ByteBuffer`.
> So, to create a direct `IntBuffer`, clients have to first create a direct 
> `ByteBuffer` then to view that buffer as an `IntBuffer`.
> 
> In other words, `IntBuffer` and friends are not first-class citizens in the 
> `Buffer` API. As such it would not be possible to map many memory segments 
> into an `IntBuffer`; in fact, the only segment we could safely map into an 
> `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it 
> doesn't seem worth adding a lot of API surface (in terms of additional 
> overloads) for such a corner case.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 88:

> 86:  * if it is associated with a confined session, it can only be accessed 
> by the thread which owns the memory session.
> 87:  * 
> 88:  * Heap segments are always associated with the {@linkplain 
> MemorySession#global() global} memory session.

I've tweaked this text, as I realized the old version was erroneously 
suggesting that all buffer segments were backed by a global session.

src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java 
line 577:

> 575: }
> 576: 
> 577: private static int getScaleFactor(Buffer buffer) {

Note: for each buffer kind, there are three possible cases. Consider 
`IntBuffer`: an `IntBuffer` can be backed by:
* `byte[]`, if it's a byte buffer view
* `int[]`, if it's allocated with `IntBuffer.allocate`, or `IntBuffer.wrap`
* null, if it comes from a direct byte buffer viewed as an `IntBuffer`

As such, the buffer kind (IntBuffer vs. LongBuffer) determines the buffer 
element size, and therefore how much the buffer position and size should be 
scaled.

But to check whether we need to create a heap vs. off-heap vs. mapped segment, 
or to check _which_ kind of heap segment we need, we need to look into the 
buffer base object, since there can be multiple options for any single buffer 
type.

-

PR: https://git.openjdk.java.net/jdk/pull/8701


RFR: 8286715: Generalize MemorySegment::ofBuffer

2022-05-13 Thread Maurizio Cimadamore
This patch makes MemorySegment::ofBuffer more general, by allowing clients to 
pass *any* `Buffer` instance, not just `ByteBuffer`.
This allows us to match expressiveness of JNI API, where JNI clients can obtain 
the address of any direct buffer instance, using the `GetDirectBufferAddress` 
function.

We thought about also providing a more general way to view a segment as a 
buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct buffers 
can only created form `ByteBuffer`.
So, to create a direct `IntBuffer`, clients have to first create a direct 
`ByteBuffer` then to view that buffer as an `IntBuffer`.

In other words, `IntBuffer` and friends are not first-class citizens in the 
`Buffer` API. As such it would not be possible to map many memory segments into 
an `IntBuffer`; in fact, the only segment we could safely map into an 
`IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it doesn't 
seem worth adding a lot of API surface (in terms of additional overloads) for 
such a corner case.

-

Commit messages:
 - Tweak javadoc
 - Merge branch 'foreign-preview' into generalize_ofbuffer
 - Merge branch 'master' into foreign-preview
 - Merge branch 'master' into foreign-preview
 - Merge branch 'master' into foreign-preview
 - Fix crashes in heap segment benchmarks due to misaligned access
 - Merge branch 'master' into foreign-preview
 - Update src/java.base/share/classes/jdk/internal/reflect/Reflection.java
 - Initial push
 - Add tests for loaderLookup/restricted method corner cases
 - ... and 59 more: https://git.openjdk.java.net/jdk/compare/11fa03f3...02494e2f

Changes: https://git.openjdk.java.net/jdk/pull/8701/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8701=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8286715
  Stats: 95 lines in 8 files changed: 55 ins; 1 del; 39 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8701/head:pull/8701

PR: https://git.openjdk.java.net/jdk/pull/8701