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

[Jorn Vernee]

On Wed, 16 Sep 2020 16:17:30 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.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address review comments

Marked as reviewed by jvernee (Committer).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java line 341:

> 339:                     try {
> 340:                         runnable.run();
> 341:                     } catch (Throwable t) {

Maybe the exception should be re-thrown after doing critical cleanup?

test/jdk/java/foreign/TestNoForeignUnsafeOverride.java line 43:

> 41:     @Test(expectedExceptions = IllegalAccessError.class)
> 42:     public void testUnsafeAccess() {
> 43:         MemorySegment.ofNativeRestricted();

Should be:
Suggestion:

        MemoryAddress.ofLong(42).asSegmentRestricted(10);
Right? To retain the same behaviour?

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

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