[foreign-memaccess] RFR: 8253243: Investigate ways to make MemorySegment::ofNativeRestricted more composable

[Paul Sandoz]

On Wed, 16 Sep 2020 14:51:24 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This patch simplifies the MemorySegment::ofNativeRestricted API, based on a number of observations:
> 
> * now that we have shared segment support, passing an explicit owner thread isn't that useful (e.g. this is no longer an
>   escape hatch, and can - and should - be done with public, safe API)
> * if we had a public safe API to add cleanup action to existing segments, we could also get rid of the cleanup parameter
> * if a custom cleanup action embedded some other object, that object will be kept reachable, so no attachment API is
>   needed
> * if we moved the method to MemoryAddress, we would streamline uses of this API quite a bit, as in
>   `address.asSegmentRestricted(100)` - e.g. only size is needed
> 
> Adding the new API for attaching cleanup action is relatively straightforward - as usual we have to dup the segment and
> the scope, and create a new segment, with a new scope whose cleanup action calls the custom runnable before the
> original action (that original action should still be called, since otherwise we would fail to free/unmap memory). Any
> exception thrown by the cleanup action is caught and ignored, so as not to mess with the critical cleanup which has to
> occur.  A nice consequence of this work is that the surface for unsafety is much reduced, and the various methods
> compose much better. For instance, the "everything" segment can be expressed as follows:  MemoryAddress.NULL
>     .asSegmentRestricted(Long.MAX_VALUE)
>     .withOwnerThread(null)
>     .withAccessModes(READ | WRITE);
> 
> Which is, I think, much nicer. The changes in tests also suggest that the new API is easier on clients, and in one case
> (TestCleaner) the need for opting into unsafe operation actually vanished.
> I also did a bit of cleaning on the javadoc, which appeared to be off-sync when it comes to confinement.

Marked as reviewed by psandoz (Committer).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java line 111:

> 109:             throw new IllegalArgumentException("Invalid size : " + bytesSize);
> 110:         }
> 111:         Utils.checkRestrictedAccess("MemoryAddress.ofSegmentRestricted");

Suggest this be the first check, so it fails independently of the arguments when permission is not granted.

Might be worth stating that closing does not free up any memory or resources. We can refer to the use of
`withCleanupAction` if such action is required.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 271:

> 269:      * but with a different cleanup action. More specifically, the cleanup action associated with the returned
> segment will 270:      * first call the user-provided action, before delegating back to the original cleanup action
> associated with 271:      * this segment. Any exception thrown by the user-provided action will be discarded, and will
> not prevent the

"this segment (if any). Any ..." ?

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java line 275:

> 273:      * <p>
> 274:      * As a side-effect, this segment will be marked as <em>not alive</em>,
> 275:      * and subsequent operations on this segment will result in runtime errors.

errors/exceptions?

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/328