8296417: Make memory session a pure lifetime abstraction #750

mcimadamore · 2022-11-04T17:14:56Z

Add description

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8296417: Make memory session a pure lifetime abstraction

Reviewers

Jorn Vernee (@JornVernee - Committer) ⚠️ Review applies to 0809daa7
Per Minborg (@minborg - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign pull/750/head:pull/750
$ git checkout pull/750

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 750

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/750.diff

bridgekeeper · 2022-11-04T17:17:20Z

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-memaccess+abi 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-11-04T17:43:38Z

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

+ * Adding new resources to the global session, does nothing: as the session can never become not-alive, there is nothing to track.
+ * Acquiring and or releasing a memory session similarly does nothing.
+ */
+final class GlobalSession extends MemorySessionImpl {


I've decided to split this (and implicit sessions) up, for better clarity

mcimadamore · 2022-11-04T17:47:42Z

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

-     * Creates an arena-based allocator used to allocate native segments. The returned allocator features
-     * the given block size {@code B} and the given arena size {@code A}, and the native segments
-     * it allocates are associated with the provided memory session.
+     * Returns a segment allocator which responds to allocation requests by returning consecutive slices


Note: newNativeArena is now gone, and replaced with a more general "slicing allocator". The reason I did this was two-fold: the arena factory seemed to be fairly specific, and not very useful to build upon (slicing allocator greatly improve on that aspect). The second problem is the name: now that we have a toplevel Arena, I think having an arena allocator factory here would be too confusing. I don't think the loss in functionality is significant (newNativeArena can be trivially implemented in terms of a slicing allocator), but if we really need it, we could always add another Arena subclass (e.g. SlicingArena which does more or less what the old allocator did.

mlbridge · 2022-11-04T18:05:45Z

Webrevs

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

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

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

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

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

test/jdk/java/foreign/SafeFunctionAccessTest.java

test/jdk/java/foreign/TestSegmentAllocators.java

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

test/jdk/java/foreign/TestMemorySession.java

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

rsmogura · 2022-11-06T14:40:56Z

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

+    }
+
+    /**
+     * {@return the session associated with this arena}


Minor, curly bracers around @return

That's the inline javadoc return tag?

minborg · 2022-11-07T09:22:24Z

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

     */
-    static MemorySegment allocateNative(long byteSize, long byteAlignment) {
+    static MemorySegment allocateNative(long byteSize, long byteAlignment, MemorySession session) {
+        Objects.requireNonNull(session);


Swap checking order

Some of our tests (TestNulls) rely on nulls being checked first. I'm ok with swapping when there are two null checks (as in allocateNative(MemoryLayout, MemorySession)).

minborg · 2022-11-07T09:24:55Z

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

-     * exceeds the arena size {@code A}, or the system capacity. Furthermore, the returned allocator is not thread safe.
-     * Concurrent allocation needs to be guarded with synchronization primitives.
+     * When the returned allocator cannot satisfy an allocation request, e.g. because a slice of the provided
+     * segment with the requested size cannot be found, an {@link IndexOutOfBoundsException} is thrown.


This type of exception appears a bit strange to me.

Given that the implementation of this method is based on MemorySegment::asSlice, I think the exception type is not too bad.

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

openjdk · 2022-11-07T13:46: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:

8296417: Make memory session a pure lifetime abstraction

Reviewed-by: jvernee, pminborg

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-memaccess+abi 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-memaccess+abi branch, type /integrate in a new comment.

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>

mcimadamore · 2022-11-07T14:50:25Z

/integrate

openjdk · 2022-11-07T14:53:03Z

Going to push as commit 2c9ab7c.

openjdk · 2022-11-07T14:53:25Z

@mcimadamore Pushed as commit 2c9ab7c.

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

merykitty · 2022-11-09T03:31:33Z

I think that Arena is a bad name. And if I read the document correctly, MemorySession is now a bad name, too, since essentially a MemorySession only allows read operations, it acts more like a view than a true session. Thanks a lot.

mcimadamore · 2022-11-09T09:35:43Z

I think that Arena is a bad name. And if I read the document correctly, MemorySession is now a bad name, too, since essentially a MemorySession only allows read operations, it acts more like a view than a true session. Thanks a lot.

Names can be changed. I agree that MemorySession seems suboptimal now. We were thinking of other candidates like SegmentScope.

That said, I'd like to understand more where the problem is with Arena, can you please expand on why you think it's not a great name? Thanks.

merykitty · 2022-11-09T10:07:21Z

@mcimadamore I think Arena is a word indicating mainly spacial properties, using it as a pure temporal abstraction seems unrelated and potentially misleading.

Moreover, in memory contexts, an arena is often used to indicate a contiguous piece of memory that can be sliced out to satisfy requested allocations. As a result, using it for an orthogonal responsibility may be confusing.

Thank you very much.

merykitty · 2022-11-09T10:19:36Z

Personally I think we could use MemoryContext or reuse the existing MemorySession name instead.

mcimadamore · 2022-11-09T10:30:38Z

@mcimadamore I think Arena is a word indicating mainly spacial properties, using it as a pure temporal abstraction seems unrelated and potentially misleading.

But the pure temporal abstraction is MemorySession, not Arena (hence why you need to go from Arena to a session to e.g. map a segment).

Moreover, in memory contexts, an arena is often used to indicate a contiguous piece of memory that can be sliced out to satisfy requested allocations. As a result, using it for an orthogonal responsibility may be confusing.

Ok, so your angle is: Arena is an allocator with very specific properties. I think that's fair - although Arena is also an allocator - and preserves a main properties or arena allocators - which is that when the arena is closed, all the memory is invalidated at once. It seems like the fact that the arena is contiguous or not under the hood (which is an impl change which we might in fact improve over time) should not be too much of a distraction?

Thank you very much.

merykitty · 2022-11-09T14:47:41Z

My view is that an Arena is the temporal abstraction of memory, as it can create and free memory. On the other hand, a MemorySession is merely a view on its owning Arena, allowing queries of the arena's status. An implicit session can be understood as a session that has an implicit Arena.

While I agree with your definition of an arena from the theoretical point of view, practically, an arena allocator is often used to indicate a more specific kind of allocator and the arena is often used to indicate the underlying segment that gets sliced. For example, protobuf C++ API specifies an Arena as: 1

With arena allocation, new objects are allocated out of a large piece of preallocated memory called the arena.

In contrast, in C++ standard, to specify a kind of memory source that "releases the allocated memory only when the resource is destroyed." the standard calls it std::pmr::monotonic_buffer_resource. 2

Thanks a lot.

JornVernee · 2022-11-09T16:38:31Z

Arena in HotSpot is like monotonic_buffer_resource, i.e. a linked list of slabs that grows over time, where the slabs get sliced into pieces to fulfill allocation requests. But in general I understand Arena to just be something that groups multiple allocations. The particular allocation strategy seems less important to understanding what an arena is than the grouping property.

PaulSandoz · 2022-11-09T16:39:51Z

We are using "Arena" to generally convey "a particular area of activity" related to memory management e.g. the activities of 1) allocation of segments associated with the arena's session; and 2) deallocation of all allocated segments, within the area of the try-with-resources body.

mcimadamore · 2022-11-09T16:45:02Z

My view is that an Arena is the temporal abstraction of memory, as it can create and free memory. On the other hand, a MemorySession is merely a view on its owning Arena, allowing queries of the arena's status. An implicit session can be understood as a session that has an implicit Arena.

Some sessions are associated with arenas, some are not (global and implicit). In my mental model, the current session is more of an access control bit on the side of a memory segment, which is consulted prior to a memory access operation.

So, IMHO the real issue here is (1) the fact that coming from a world where there's only MemorySession this new shift seems surprising and (2), a toplevel MemorySession abstraction is probably attracting more attention than it should at this point. But these seem things that could be addressed with some naming/javadoc changes.

mcimadamore force-pushed the session_drop_min branch from 764383b to b7010bd Compare November 4, 2022 17:24

Initial push

6d5898d

mcimadamore force-pushed the session_drop_min branch from b7010bd to 6d5898d Compare November 4, 2022 17:42

mcimadamore commented Nov 4, 2022

View reviewed changes

mcimadamore added 2 commits November 4, 2022 17:51

Add missing copyrights

1c91660

Fix stale references to MemorySession

00ec10b

mcimadamore changed the title ~~Make memory sessions pure lifetime abstractions~~ 8296417: Make memory session a pure lifetime abstraction Nov 4, 2022

mcimadamore marked this pull request as ready for review November 4, 2022 17:59

openjdk bot added the rfr Ready for review label Nov 4, 2022

Tweak javadoc for VaList

8ee48e8

rsmogura reviewed Nov 4, 2022

View reviewed changes

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

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

Fix vector tests

9f390bf

JornVernee reviewed Nov 4, 2022

View reviewed changes

mcimadamore added 3 commits November 4, 2022 22:27

Address review comments

a780109

Unseal Arena

3183bfc

Fix SegmentAllocator.slicingAllocator to throw IIOBE

a81734f

JornVernee reviewed Nov 4, 2022

View reviewed changes

test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java Outdated Show resolved Hide resolved

rsmogura reviewed Nov 6, 2022

View reviewed changes

minborg reviewed Nov 7, 2022

View reviewed changes

Address more review comments

0809daa

JornVernee approved these changes Nov 7, 2022

View reviewed changes

JornVernee reviewed Nov 7, 2022

View reviewed changes

test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java Outdated Show resolved Hide resolved

openjdk bot added the ready Ready to be integrated label Nov 7, 2022

Update test/micro/org/openjdk/bench/java/lang/foreign/StrLenTest.java

e08af8b

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>

minborg approved these changes Nov 7, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 7, 2022

openjdk bot closed this Nov 7, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296417: Make memory session a pure lifetime abstraction #750

8296417: Make memory session a pure lifetime abstraction #750

mcimadamore commented Nov 4, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 4, 2022

mcimadamore Nov 4, 2022

mcimadamore Nov 4, 2022 •

edited

mlbridge bot commented Nov 4, 2022 •

edited

rsmogura Nov 6, 2022

mcimadamore Nov 7, 2022

minborg Nov 7, 2022

mcimadamore Nov 7, 2022

minborg Nov 7, 2022

mcimadamore Nov 7, 2022

openjdk bot commented Nov 7, 2022 •

edited

mcimadamore commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

merykitty commented Nov 9, 2022

mcimadamore commented Nov 9, 2022

merykitty commented Nov 9, 2022 •

edited

merykitty commented Nov 9, 2022

mcimadamore commented Nov 9, 2022

merykitty commented Nov 9, 2022 •

edited

JornVernee commented Nov 9, 2022

PaulSandoz commented Nov 9, 2022 •

edited

mcimadamore commented Nov 9, 2022

8296417: Make memory session a pure lifetime abstraction #750

8296417: Make memory session a pure lifetime abstraction #750

Conversation

mcimadamore commented Nov 4, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 4, 2022

mcimadamore Nov 4, 2022

Choose a reason for hiding this comment

mcimadamore Nov 4, 2022 • edited

Choose a reason for hiding this comment

mlbridge bot commented Nov 4, 2022 • edited

Webrevs

rsmogura Nov 6, 2022

Choose a reason for hiding this comment

mcimadamore Nov 7, 2022

Choose a reason for hiding this comment

minborg Nov 7, 2022

Choose a reason for hiding this comment

mcimadamore Nov 7, 2022

Choose a reason for hiding this comment

minborg Nov 7, 2022

Choose a reason for hiding this comment

mcimadamore Nov 7, 2022

Choose a reason for hiding this comment

openjdk bot commented Nov 7, 2022 • edited

mcimadamore commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

openjdk bot commented Nov 7, 2022

merykitty commented Nov 9, 2022

mcimadamore commented Nov 9, 2022

merykitty commented Nov 9, 2022 • edited

merykitty commented Nov 9, 2022

mcimadamore commented Nov 9, 2022

merykitty commented Nov 9, 2022 • edited

JornVernee commented Nov 9, 2022

PaulSandoz commented Nov 9, 2022 • edited

mcimadamore commented Nov 9, 2022

mcimadamore commented Nov 4, 2022 •

edited by openjdk bot

mcimadamore Nov 4, 2022 •

edited

mlbridge bot commented Nov 4, 2022 •

edited

openjdk bot commented Nov 7, 2022 •

edited

merykitty commented Nov 9, 2022 •

edited

merykitty commented Nov 9, 2022 •

edited

PaulSandoz commented Nov 9, 2022 •

edited