8281855: Rename ResourceScope to MemorySession #641

mcimadamore · 2022-02-15T17:32:50Z

This patch implements some of the changes described in [1] and [2]. More specifically, the ResourceScope abstraction is renamed into MemorySession, and the session accessors in MemorySegment, NativeSymbol and VaList are tweaked to return non-closeable session views. To counteract this change, MemorySession now supports equals and hashCode, so that sessions views can be compared to each other in ways that are not identity-sensitive.

[1] - https://mail.openjdk.java.net/pipermail/panama-dev/2022-February/016152.html
[2] - https://mail.openjdk.java.net/pipermail/panama-dev/2022-February/016254.html

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed

Issue

JDK-8281855: Rename ResourceScope to MemorySession

Reviewers

Paul Sandoz (@PaulSandoz - Committer) ⚠️ Review applies to 2ee6f16
Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to d1fc22b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/641/head:pull/641
$ git checkout pull/641

Update a local copy of the PR:
$ git checkout pull/641
$ git pull https://git.openjdk.java.net/panama-foreign pull/641/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 641

View PR using the GUI difftool:
$ git pr show -t 641

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/641.diff

bridgekeeper · 2022-02-15T17:34:04Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-preview will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mcimadamore · 2022-02-15T17:39:07Z

src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java

+     * accessors (see {@link MemorySegment#session()}). This class forwards all session methods to the underlying
+     * "root" session implementation, and throws {@link UnsupportedOperationException} on close.
+     */
+    public final static class NonCloseableView implements MemorySession, Scoped {


This is a simple wrapper class around a session which delegates everything to the session from which it is built - and overrides close to throw an exception. I believe the cost for creating these little instances will be negligible - if problems show up, we can always move to more complex approaches (e.g. where MemorySegment implements MemorySession, or where we use @Stable fields to cache a non-closeable view inside the root memory session).

mcimadamore · 2022-02-15T17:40:51Z

src/java.base/share/classes/jdk/internal/foreign/Scoped.java


 public interface Scoped {
-    ResourceScope scope();
+    MemorySessionImpl sessionImpl();


This method is used to retrieve the "root" session associated with either a resource, or a session view. This means that, to simplify the implementation, MemorySessionImpl has to implement Scoped as well (and return itself).

mcimadamore · 2022-02-15T17:43:24Z

I've fixed javadoc references to "scopes" and improved the javadoc text in several places. A link to a javadoc corresponding to these changes can be found here:
http://cr.openjdk.java.net/~mcimadamore/panama/session_preview/javadoc/java.base/java/lang/foreign/package-summary.html

mlbridge · 2022-02-15T17:51:44Z

Webrevs

mlbridge · 2022-02-16T22:08:41Z

Mailing list message from Maurizio Cimadamore on panama-dev:

That would be an implicit session. Implicit sessions are back to being
non-closeable, and shared, in this patch.

Maurizio

merykitty · 2022-02-17T23:39:55Z

src/java.base/share/classes/java/lang/foreign/MemorySegment.java

     */
-    ResourceScope scope();
+    MemorySession session();


Hi, could we take advantage of the type system and return a MemorySessionView which does not have a close method on it. Thanks.

This is a possibility which we have explored. In the end we decided against it because we felt splitting session/scope in two would have had a negative effect on API discoverability. Note that non-closeable sessions are not just used for e.g. MemorySegment::session, but they are also returned by MemorySession::global and MemorySession::openImplicit. If we ever do structured sessions (e.g. session that give access to any threads that inherit a given Loom scope local), then it is likely that these sessions will be non-closeable too.

Also, you would need to replace most API methods accepting MemorySession with MemorySessionView - and add a subtyping relationhip between MemorySessionView and MemorySession (so that you can pass a MemorySession where a view is expected). Ultimately that all felt a bit too much.

Ultimately that all felt a bit too much.

To be clear, it felt too much not because the refactoring was complex (it was not) but because this creates a problem of how do you name the non-closeable entity (MemorySessionView is fine when returned by MemorySegment::session, less fine when accepted by MemorySegment::allocateNative). And if you give it a more general name, then you end up with two abstractions fighting for API visibility.

I see, rethinking about it, the reason to add a session accessor to MemorySegment was to properly bind the lifetime of an allocated segment to an existing one and to correctly observe that lifetime. It sounds like the functionality of an allocator to me. Maybe we could instead return a SegmentAllocator instead (and change MemorySession to SegmentDeallocator probably) and extend the functionality of a SegmentAllocator to check the validity of its associating MemorySession. Even if it is not the case we would need to check the state of a SegmentAllocator anyway to ensure that we can allocate memory with it.
Thanks.

That option was also considered, but discarded because it would make SegmentAllocator stateful, e.g. more complex than just a functional interface. Right now it is pretty easy to define a new allocator - but if the allocator has to track lifetime, it becomes harder to define custom ones. Also, saying that, to create an upcall stub you need a SegmentAllocator, or that to create an unsafe segment view of a memory address (MemorySegment::ofAddress) you need an allocator, is a bit odd - in the sense that you don't really need allocation capabilities there - you just need some kind of lifetime abstraction.

So, I think the "allocation" angle is valid in most cases, but I think that it only works well for native segments and doesn't really generalize to all segments - unsafe views, memory mapped segments, heap segments, etc.

I think separating lifecycle from allocation was a really powerful move, which paid off considerably. Having a single allocator abstraction perform double duty seems a step backwards IMHO.

I see, rethinking about it, the reason to add a session accessor to MemorySegment was to properly bind the lifetime of an allocated segment to an existing one and to correctly observe that lifetime. It sounds like the functionality of an allocator to me. Maybe we could instead return a SegmentAllocator instead (and change MemorySession to SegmentDeallocator probably) and extend the functionality of a SegmentAllocator to check the validity of its associating MemorySession. Even if it is not the case we would need to check the state of a SegmentAllocator anyway to ensure that we can allocate memory with it. Thanks.

hat option was also considered, but discarded because it would make SegmentAllocator stateful, e.g. more complex than just a functional interface.

Note that today there is no contract which says that all segments returned by a SegmentAllocator must have the same session. While some allocators might behave like that, I think it would not be ok to force that behavior on all allocators. And, we considered to add, perhaps in the future, some small subinterface, like ScopedSegmentAllocator which also has a session accessor - this would be a good return type for the arena allocator methods, for instance. But this is a move that is orthogonal to the changes described in this PR (and also not a binding one - we can add the sub-interface even at a later point w/o breaking compatibility).

Thanks a lot for your clarification, I still think that having a scope that throw on close() sounds a lot like the problem with UnmodifiableCollection but it seems the alternatives have been considered already.

Indeed the issues are similar. However, there are some characteristics that are unique to this API. MemorySegment is a lower-level API than, say Collection. Also, while we currently do not generate method handles which require a MemorySession, adding intricate subtyping hierarchies always has a cost when interacting with invokeExact - so this API often errs on the side of "avoid unnecessary intermediate types, if we can avoid it". Not to dismiss your concern, which is a valid one - just wanted to explain some of the rationale behind some of the API decisions we've been doing.

src/java.base/share/classes/java/lang/ClassLoader.java

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template

openjdk · 2022-02-18T19:27:39Z

@mcimadamore this pull request can not be integrated into foreign-preview due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout session_preview
git fetch https://git.openjdk.java.net/panama-foreign foreign-preview
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge foreign-preview"
git push

…late Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

* replace spurious usages of `scope` in direct buffer template

openjdk · 2022-02-21T11:37:14Z

@mcimadamore This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8281855: Rename ResourceScope to MemorySession

Reviewed-by: psandoz, jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the foreign-preview branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-preview branch, type /integrate in a new comment.

src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java

JornVernee · 2022-02-16T21:30:31Z

src/java.base/share/classes/jdk/internal/foreign/abi/UpcallStubs.java

            @Override
            public void cleanup() {
                freeUpcallStub(entry);
            }
        });
-        return new NativeSymbolImpl("upcall:" + Long.toHexString(entry), MemoryAddress.ofLong(entry), scope);
+        return new NativeSymbolImpl("upcall:" + Long.toHexString(entry), MemoryAddress.ofLong(entry), ((MemorySessionImpl)session));


If I understand correctly, this doesn't use Scoped.toSessionImpl since the session doesn't need to be closed in this case?

No: NativeSymbolImpl wants a true, closeable MemorySessionImpl. Then, if you look at that class, its session method will return a non closeable view of that session. The toSessionImpl happens in AbstractCLinker::upcallStub.

I see. I think that's fine, but maybe that call should be moved here for clarity? (or maybe just add an assert session.isClosable() for documentation).

Thanks, the comment helps :)

test/jdk/java/foreign/SafeFunctionAccessTest.java

test/jdk/java/foreign/TestRestricted.java

src/java.base/share/classes/java/lang/foreign/MemorySession.java

JornVernee · 2022-02-21T13:22:18Z

test/jdk/java/foreign/valist/VaListTest.java

    private static final Function<Consumer<VaList.Builder>, VaList> winVaListFactory
-            = actions -> Windowsx64Linker.newVaList(actions, ResourceScope.newConfinedScope());
+            = actions -> Windowsx64Linker.newVaList(actions, MemorySession.openConfined());
    private static final Function<Consumer<VaList.Builder>, VaList> sysvVaListFactory
-            = actions -> SysVx64Linker.newVaList(actions, ResourceScope.newConfinedScope());
+            = actions -> SysVx64Linker.newVaList(actions, MemorySession.openConfined());
    private static final Function<Consumer<VaList.Builder>, VaList> linuxAArch64VaListFactory
-            = actions -> LinuxAArch64Linker.newVaList(actions, ResourceScope.newConfinedScope());
+            = actions -> LinuxAArch64Linker.newVaList(actions, MemorySession.openConfined());
    private static final Function<Consumer<VaList.Builder>, VaList> macAArch64VaListFactory
-            = actions -> MacOsAArch64Linker.newVaList(actions, ResourceScope.newConfinedScope());
+            = actions -> MacOsAArch64Linker.newVaList(actions, MemorySession.openConfined());
    private static final Function<Consumer<VaList.Builder>, VaList> platformVaListFactory
-            = (builder) -> VaList.make(builder, ResourceScope.newConfinedScope());
-
-    private static final BiFunction<Consumer<VaList.Builder>, ResourceScope, VaList> winVaListScopedFactory
-            = (builder, scope) -> Windowsx64Linker.newVaList(builder, scope);
-    private static final BiFunction<Consumer<VaList.Builder>, ResourceScope, VaList> sysvVaListScopedFactory
-            = (builder, scope) -> SysVx64Linker.newVaList(builder, scope);
-    private static final BiFunction<Consumer<VaList.Builder>, ResourceScope, VaList> linuxAArch64VaListScopedFactory
-            = (builder, scope) -> LinuxAArch64Linker.newVaList(builder, scope);
-    private static final BiFunction<Consumer<VaList.Builder>, ResourceScope, VaList> macAArch64VaListScopedFactory
-            = (builder, scope) -> MacOsAArch64Linker.newVaList(builder, scope);
-    private static final BiFunction<Consumer<VaList.Builder>, ResourceScope, VaList> platformVaListScopedFactory
-            = (builder, scope) -> VaList.make(builder, scope);
+            = (builder) -> VaList.make(builder, MemorySession.openConfined());


Now that the tests no longer close the scope, maybe these scopes should be implicit scopes? (at least then we still test the eventual closure of the VaList).

test/micro/org/openjdk/bench/java/lang/foreign/BulkMismatchAcquire.java

mcimadamore · 2022-02-21T15:45:58Z

/integrate

openjdk · 2022-02-21T15:47:11Z

Going to push as commit 9214de4.

openjdk · 2022-02-21T15:47:31Z

@mcimadamore Pushed as commit 9214de4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

mcimadamore added 4 commits February 11, 2022 14:51

Initial push

61247ea

Fix MemorySession javadoc

c8f19ff

Fix reference to ResourceScope in javadoc

ae43169

Merge branch 'foreign-preview' into session_preview

473f328

mcimadamore commented Feb 15, 2022

View reviewed changes

Fix whitespaces

0d779de

mcimadamore marked this pull request as ready for review February 15, 2022 17:46

openjdk bot added the rfr Ready for review label Feb 15, 2022

merykitty reviewed Feb 17, 2022

View reviewed changes

Fix more spurious usages of ResourceScope

d9f36be

PaulSandoz approved these changes Feb 18, 2022

View reviewed changes

src/java.base/share/classes/java/lang/ClassLoader.java Outdated Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/access/JavaNioAccess.java Show resolved Hide resolved

src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template Outdated Show resolved Hide resolved

openjdk bot added the merge-conflict label Feb 18, 2022

mcimadamore and others added 3 commits February 18, 2022 21:27

Update src/java.base/share/classes/java/nio/Direct-X-Buffer.java.temp…

d916ad5

…late Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

Update src/java.base/share/classes/java/lang/ClassLoader.java

d3a6c44

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

Address review comments

2ee6f16

* replace spurious usages of `scope` in direct buffer template

PaulSandoz approved these changes Feb 19, 2022

View reviewed changes

Merge branch 'foreign-preview' into session_preview

3b33c66

openjdk bot added ready Ready to be integrated and removed merge-conflict labels Feb 21, 2022

Fix spurious reference to ResourceScopeImpl after merge

bf979ed

JornVernee reviewed Feb 21, 2022

View reviewed changes

mcimadamore added 2 commits February 21, 2022 14:57

Merge branch 'foreign-preview' into session_preview

bee07da

Address review comments

d1fc22b

JornVernee approved these changes Feb 21, 2022

View reviewed changes

Add a comment in UpcallStubs

1b10439

openjdk bot added the integrated Pull request has been integrated label Feb 21, 2022

openjdk bot closed this Feb 21, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8281855: Rename ResourceScope to MemorySession #641

8281855: Rename ResourceScope to MemorySession #641

mcimadamore commented Feb 15, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 15, 2022

mcimadamore Feb 15, 2022

mcimadamore Feb 15, 2022

mcimadamore commented Feb 15, 2022

mlbridge bot commented Feb 15, 2022 •

edited

Loading

mlbridge bot commented Feb 16, 2022

merykitty Feb 17, 2022

mcimadamore Feb 18, 2022

mcimadamore Feb 18, 2022 •

edited

Loading

merykitty Feb 18, 2022

mcimadamore Feb 18, 2022

mcimadamore Feb 18, 2022

merykitty Feb 18, 2022

mcimadamore Feb 18, 2022

openjdk bot commented Feb 18, 2022

openjdk bot commented Feb 21, 2022 •

edited

Loading

JornVernee Feb 16, 2022

mcimadamore Feb 21, 2022

JornVernee Feb 21, 2022 •

edited

Loading

JornVernee Feb 21, 2022

JornVernee Feb 21, 2022 •

edited

Loading

mcimadamore commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

8281855: Rename ResourceScope to MemorySession #641

8281855: Rename ResourceScope to MemorySession #641

Conversation

mcimadamore commented Feb 15, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore commented Feb 15, 2022

mlbridge bot commented Feb 15, 2022 • edited Loading

Webrevs

mlbridge bot commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 18, 2022

openjdk bot commented Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

mcimadamore commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

openjdk bot commented Feb 21, 2022

mcimadamore commented Feb 15, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 15, 2022 •

edited

Loading

mcimadamore Feb 18, 2022 •

edited

Loading

openjdk bot commented Feb 21, 2022 •

edited

Loading

JornVernee Feb 21, 2022 •

edited

Loading

JornVernee Feb 21, 2022 •

edited

Loading