Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8254162: Implementation of Foreign-Memory Access API (Third Incubator) #548

Closed
wants to merge 36 commits into from

Conversation

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Oct 7, 2020

This patch contains the changes associated with the third incubation round of the foreign memory access API incubation (see JEP 393 [1]). This iteration focus on improving the usability of the API in 3 main ways:

  • first, by providing a way to obtain truly shared segments, which can be accessed and closed concurrently from multiple threads
  • second, by providing a way to register a memory segment against a Cleaner, so as to have some (optional) guarantee that the memory will be deallocated, eventually
  • third, by not requiring users to dive deep into var handles when they first pick up the API; a new MemoryAccess class has been added, which defines several useful dereference routines; these are really just thin wrappers around memory access var handles, but they make the barrier of entry for using this API somewhat lower.

A big conceptual shift that comes with this API refresh is that the role of MemorySegment and MemoryAddress is not the same as it used to be; it used to be the case that a memory address could (sometimes, not always) have a back link to the memory segment which originated it; additionally, memory access var handles used MemoryAddress as a basic unit of dereference.

This has all changed as per this API refresh; now a MemoryAddress is just a dumb carrier which wraps a pair of object/long addressing coordinates; MemorySegment has become the star of the show, as far as dereferencing memory is concerned. You cannot dereference memory if you don't have a segment. This improves usability in a number of ways - first, it is a lot easier to wrap native addresses (long, essentially) into a MemoryAddress; secondly, it is crystal clear what a client has to do in order to dereference memory: if a client has a segment, it can use that; otherwise, if the client only has an address, it will have to create a segment unsafely (this can be done by calling MemoryAddress::asSegmentRestricted).

A list of the API, implementation and test changes is provided below. If you have any questions, or need more detailed explanations, I (and the rest of the Panama team) will be happy to point at existing discussions, and/or to provide the feedback required.

A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom the work on shared memory segment would not have been possible; also I'd like to thank Paul Sandoz, whose insights on API design have been very helpful in this journey.

Thanks
Maurizio

Javadoc:

http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html

Specdiff:

http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html

CSR:

https://bugs.openjdk.java.net/browse/JDK-8254163

API Changes

  • MemorySegment
    • drop factory for restricted segment (this has been moved to MemoryAddress, see below)
    • added a no-arg factory for a native restricted segment representing entire native heap
    • rename withOwnerThread to handoff
    • add new share method, to create shared segments
    • add new registerCleaner method, to register a segment against a cleaner
    • add more helpers to create arrays from a segment e.g. toIntArray
    • add some asSlice overloads (to make up for the fact that now segments are more frequently used as cursors)
    • rename baseAddress to address (so that MemorySegment can implement Addressable)
  • MemoryAddress
    • drop segment accessor
    • drop rebase method and replace it with segmentOffset which returns the offset (a long) of this address relative to a given segment
  • MemoryAccess
    • New class supporting several static dereference helpers; the helpers are organized by carrier and access mode, where a carrier is one of the usual suspect (a Java primitive, minus boolean); the access mode can be simple (e.g. access base address of given segment), or indexed, in which case the accessor takes a segment and either a low-level byte offset,or a high level logical index. The classification is reflected in the naming scheme (e.g. getByte vs. getByteAtOffset vs getByteAtIndex).
  • MemoryHandles
    • drop withOffset combinator
    • drop withStride combinator
    • the basic memory access handle factory now returns a var handle which takes a MemorySegment and a long - from which it is easy to derive all the other handles using plain var handle combinators.
  • Addressable
    • This is a new interface which is attached to entities which can be projected to a MemoryAddress. For now, both MemoryAddress and MemorySegment implement it; we have plans, with JEP 389 [2] to add more implementations. Clients can largely ignore this interface, which comes in really handy when defining native bindings with tools like jextract.
  • MemoryLayouts
    • A new layout, for machine addresses, has been added to the mix.

Implementation changes

There are two main things to discuss here: support for shared segments, and the general simplification of the memory access var handle support.

Shared segments

The support for shared segments cuts in pretty deep in the VM. Support for shared segments is notoriously hard to achieve, at least in a way that guarantees optimal access performances. This is caused by the fact that, if a segment is shared, it would be possible for a thread to close it while another is accessing it.

After considering several options (see [3]), we zeroed onto an approach which is inspired by an happy idea that Andrew Haley had (and that he reminded me of at this year OpenJDK committer workshop - thanks!). The idea is that if we could freeze the world (e.g. with a GC pause), while a segment is closed, we could then prevent segments from being accessed concurrently to a close operation. For this to work, it is crucial that no GC safepoints can occur between a segment liveness check and the access itself (otherwise it would be possible for the accessing thread to stop just right before an unsafe call). It also relies on the fact that hotspot/C2 should not be able to propagate loads across safepoints.

Sadly, none of these conditions seems to be valid in the current implementation, so we needed to resort to a bit of creativity. First, we noted that, if we could mark so called scoped method with an annotation, it would be very simply to check as to whether a thread was in the middle of a scoped method when we stopped the world for a close operation (btw, instead of stopping the world, we do a much more efficient, thread-local polling, thanks to JEP 312 [4]).

The question is, then, once we detect that a thread is accessing the very segment we're about to close, what should happen? We first experimented with a solution which would install an asynchronous exception on the accessing thread, thus making it fail. This solution has some desirable properties, in that a close operation always succeeds. Unfortunately the machinery for async exceptions is a bit fragile (e.g. not all the code in hotspot checks for async exceptions); to minimize risks, we decided to revert to a simpler strategy, where close might fail when it finds that another thread is accessing the segment being closed.

As written in the javadoc, this doesn't mean that clients should just catch and try again; an exception on close is a bug in the user code, likely arising from lack of synchronization, and should be treated as such.

In terms of gritty implementation, we needed to centralize memory access routines in a single place, so that we could have a set of routines closely mimicking the primitives exposed by Unsafe but which, in addition, also provided a liveness check. This way we could mark all these routines with the special @Scoped annotation, which tells the VM that something important is going on.

To achieve this, we created a new (autogenerated) class, called ScopedMemoryAccess. This class contains all the main memory access primitives (including bulk access, like copyMemory, or setMemory), and accepts, in addition to the access coordinates, also a scope object, which is tested before access. A reachability fence is also thrown in the mix to make sure that the scope is kept alive during access (which is important when registering segments against cleaners).

Of course, to make memory access safe, memory access var handles, byte buffer var handles, and byte buffer API should use the new ScopedMemoryAccess class instead of unsafe, so that a liveness check can be triggered (in case a scope is present).

ScopedMemoryAccess has a closeScope method, which initiates the thread-local handshakes, and returns true if the handshake completed successfully.

The implementation of MemoryScope (now significantly simplified from what we had before), has two implementations, one for confined segments and one for shared segments; the main difference between the two is what happens when the scope is closed; a confined segment sets a boolean flag to false, and returns, whereas a shared segment goes into a CLOSING state, then starts the handshake, and then updates the state again, to either CLOSED or ALIVE depending on whether the handshake was successful or not. Note that when a shared segment is in the CLOSING state, MemorySegment::isAlive will still return true, while the liveness check upon memory access will fail.

Memory access var handles overhaul

The key realization here was that if all memory access var handles took a coordinate pair of MemorySegment and long, all other access types could be derived from this basic var handle form.

This allowed us to remove the on-the-fly var handle generation, and to simply derive structural access var handles (such as those obtained by calling MemoryLayout::varHandle) using plain var handle combinators, so that e.g. additional offset is injected into a base memory access var handle.

This also helped in simplifying the implementation by removing the special withStride and withOffset combinators, which previously needed low-level access on the innards of the memory access var handle. All that code is now gone.

Test changes

Not much to see here - most of the tests needed to be updated because of the API changes. Some were beefed up (like the array test, since now segments can be projected into many different kinds of arrays). A test has been added to test the Cleaner functionality, and another stress test has been added for shared segments (TestHandshake). Some of the microbenchmarks also needed some tweaks - and some of them were also updated to also test performance in the shared segment case.

[1] - https://openjdk.java.net/jeps/393
[2] - https://openjdk.java.net/jeps/389
[3] - https://mail.openjdk.java.net/pipermail/panama-dev/2020-May/009004.html
[4] - https://openjdk.java.net/jeps/312


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254162: Implementation of Foreign-Memory Access API (Third Incubator)

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/548/head:pull/548
$ git checkout pull/548

mcimadamore added 2 commits Oct 7, 2020
…ator)

This patch contains the changes associated with the third incubation round of the foreign memory access API incubation  (see JEP 393 [1]). This iteration focus on improving the usability of the API in 3 main ways:

* first, by providing a way to obtain truly *shared* segments, which can be accessed and closed concurrently from multiple threads
* second, by providing a way to register a memory segment against a `Cleaner`, so as to have some (optional) guarantee that the memory will be deallocated, eventually
* third, by not requiring users to dive deep into var handles when they first pick up the API; a new `MemoryAccess` class has been added, which defines several useful dereference routines; these are really just thin wrappers around memory access var handles, but they make the barrier of entry for using this API somewhat lower.

A big conceptual shift that comes with this API refresh is that the role of `MemorySegment` and `MemoryAddress` is not the same as it used to be; it used to be the case that a memory address could (sometimes, not always) have a back link to the memory segment which originated it; additionally, memory access var handles used `MemoryAddress` as a basic unit of dereference.

This has all changed as per this API refresh;  now a `MemoryAddress` is just a dumb carrier which wraps a pair of object/long addressing coordinates; `MemorySegment` has become the star of the show, as far as dereferencing memory is concerned. You cannot dereference memory if you don't have a segment. This improves usability in a number of ways - first, it is a lot easier to wrap native addresses (`long`, essentially) into a `MemoryAddress`; secondly, it is crystal clear what a client has to do in order to dereference memory: if a client has a segment, it can use that; otherwise, if the client only has an address, it will have to create a segment *unsafely* (this can be done by calling `MemoryAddress::asSegmentRestricted`).

A list of the API, implementation and test changes is provided below. If  you have any questions, or need more detailed explanations, I (and the  rest of the Panama team) will be happy to point at existing discussions,  and/or to provide the feedback required.

A big thank to Erik Osterlund, Vladimir Ivanov and David Holmes, without whom the work on shared memory segment would not have been possible.

Thanks
Maurizio

Javadoc:

http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html

Specdiff:

http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html

CSR:

https://bugs.openjdk.java.net/browse/JDK-8254163

* `MemorySegment`
  * drop factory for restricted segment (this has been moved to `MemoryAddress`, see below)
  * added a no-arg factory for a native restricted segment representing entire native heap
  * rename `withOwnerThread` to `handoff`
  * add new `share` method, to create shared segments
  * add new `registerCleaner` method, to register a segment against a cleaner
  * add more helpers to create arrays from a segment e.g. `toIntArray`
  * add some `asSlice` overloads (to make up for the fact that now segments are more frequently used as cursors)
  * rename `baseAddress` to `address` (so that `MemorySegment` can implement `Addressable`)
* `MemoryAddress`
  * drop `segment` accessor
  * drop `rebase` method and replace it with `segmentOffset` which returns the offset (a `long`) of this address relative to a given segment
* `MemoryAccess`
  * New class supporting several static dereference helpers; the helpers are organized by carrier and access mode, where a carrier is one of the usual suspect (a Java primitive, minus `boolean`); the access mode can be simple (e.g. access base address of given segment), or indexed, in which case the accessor takes a segment and either a low-level byte offset,or a high level logical index. The classification is reflected in the naming scheme (e.g. `getByte` vs. `getByteAtOffset` vs `getByteAtIndex`).
* `MemoryHandles`
  * drop `withOffset` combinator
  * drop `withStride` combinator
  * the basic memory access handle factory now returns a var handle which takes a `MemorySegment` and a `long` - from which it is easy to derive all the other handles using plain var handle combinators.
* `Addressable`
  * This is a new interface which is attached to entities which can be projected to a `MemoryAddress`. For now, both `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP 389 [2] to add more implementations. Clients can largely ignore this interface, which comes in really handy when defining native bindings with tools like `jextract`.
* `MemoryLayouts`
  * A new layout, for machine addresses, has been added to the mix.

There are two main things to discuss here: support for shared segments, and the general simplification of the memory access var handle support.

The support for shared segments cuts in pretty deep in the VM. Support for shared segments is notoriously hard to achieve, at least in a way that guarantees optimal access performances. This is caused by the fact that, if a segment is shared, it would be possible for a thread to close it while another is accessing it.

After considering several options (see [3]), we zeroed onto an approach which is inspired by an happy idea that Andrew Haley had (and that he reminded me of at this year OpenJDK committer workshop - thanks!). The idea is that if we could *freeze* the world (e.g. with a GC pause), while a segment is closed, we could then prevent segments from being accessed concurrently to a close operation. For this to work, it  is crucial that no GC safepoints can occur between a segment liveness check and the access itself (otherwise it would be possible for the accessing thread to stop just right before an unsafe call). It also relies on the fact that hotspot/C2 should not be able to propagate loads across safepoints.

Sadly, none of these conditions seems to be valid in the current implementation, so we needed to resort to a bit of creativity. First, we noted that, if we could mark so called *scoped* method with an annotation, it would be very simply to check as to whether a thread was in the middle of a scoped method when we stopped the world for a close operation (btw, instead of stopping the world, we do a much more efficient, thread-local polling, thanks to JEP 312 [4]).

The question is, then, once we detect that a thread is accessing the very segment we're about to close, what should happen? We first experimented with a solution which would install an *asynchronous* exception on the accessing thread, thus making it fail. This solution has some desirable properties, in that a `close` operation always succeeds. Unfortunately the machinery for async exceptions is a bit fragile (e.g. not all the code in hotspot checks for async exceptions); to minimize risks, we decided to revert to a simpler strategy, where `close` might fail when it finds that another thread is accessing the segment being closed.

As written in the javadoc, this doesn't mean that clients should just catch and try again; an exception on `close` is a bug in the user code, likely arising from lack of synchronization, and should be treated as such.

In terms of gritty implementation, we needed to centralize memory access routines in a single place, so that we could have a set of routines closely mimicking the primitives exposed by `Unsafe` but which, in addition, also provided a liveness check. This way we could mark all these routines with the special `@Scoped` annotation, which tells the VM that something important is going on.

To achieve this, we created a new (autogenerated) class, called `ScopedMemoryAccess`. This class contains all the main memory access primitives (including bulk access, like `copyMemory`, or `setMemory`), and accepts, in addition to the access coordinates, also a scope object, which is tested before access. A reachability fence is also thrown in the mix to make sure that the scope is kept alive during access (which is important when registering segments against cleaners).

Of course, to make memory access safe, memory access var handles, byte buffer var handles, and byte buffer API should use the new `ScopedMemoryAccess` class instead of unsafe, so that a liveness check can be triggered (in case a scope is present).

`ScopedMemoryAccess` has a `closeScope` method, which initiates the thread-local handshakes, and returns `true` if the handshake completed successfully.

The implementation of `MemoryScope` (now significantly simplified from what we had before), has two implementations, one for confined segments and one for shared segments; the main difference between the two is what happens when the scope is closed; a confined segment sets a boolean flag to false, and returns, whereas a shared segment goes into a `CLOSING` state, then starts the handshake, and then updates the state again, to either `CLOSED` or `ALIVE` depending on whether the handshake was successful or not. Note that when a shared segment is in the `CLOSING` state, `MemorySegment::isAlive` will still return `true`, while the liveness check upon memory access will fail.

The key realization here was that if all memory access var handles took a coordinate pair of `MemorySegment` and `long`, all other access types could be derived from this basic var handle form.

This allowed us to remove the on-the-fly var handle generation, and to simply derive structural access var handles (such as those obtained by calling `MemoryLayout::varHandle`) using *plain* var handle combinators, so that e.g. additional offset is injected into a base memory access var handle.

This also helped in simplifying the implementation by removing the special `withStride` and `withOffset` combinators, which previously needed low-level access on the innards of the memory access var handle. All that code is now gone.

Not much to see here - most of the tests needed to be updated because of the API changes. Some were beefed up (like the array test, since now segments can be projected into many different kinds of arrays). A test has been added to test the `Cleaner` functionality, and another stress test has been added for shared segments (`TestHandshake`). Some of the microbenchmarks also needed some tweaks - and some of them were also updated to also test performance in the shared segment case.

[1] - https://openjdk.java.net/jeps/393
[2] - https://openjdk.java.net/jeps/389
[3] - https://mail.openjdk.java.net/pipermail/panama-dev/2020-May/009004.html
[4] - https://openjdk.java.net/jeps/312
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 7, 2020

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

@openjdk
Copy link

@openjdk openjdk bot commented Oct 7, 2020

@mcimadamore The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mcimadamore mcimadamore changed the title RFR 8254162: Implementation of Foreign-Memory Access API (Third Incubator) 8254162: Implementation of Foreign-Memory Access API (Third Incubator) Oct 7, 2020
@openjdk openjdk bot added the rfr label Oct 7, 2020
Copy link
Member

@erikj79 erikj79 left a comment

Build changes look pretty good, just a few minor nits.

Copy link
Contributor

@shipilev shipilev left a comment

Drive-by review.

src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/scopedMemoryAccess.cpp Outdated Show resolved Hide resolved
test/jdk/java/foreign/TestMismatch.java Outdated Show resolved Hide resolved
Copy link
Contributor

@shipilev shipilev left a comment

Thanks, these changes make sense to me.

@erikj79
erikj79 approved these changes Oct 8, 2020
Copy link
Member

@erikj79 erikj79 left a comment

Build changes look ok.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 8, 2020

@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:

8254162: Implementation of Foreign-Memory Access API (Third Incubator)

Reviewed-by: erikj, psandoz, alanb

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 5 new commits pushed to the master branch:

  • 19bade0: 8256238: Remove Matcher::pass_original_key_for_aes
  • f7685a4: 8256203: Simplify RegMask::Empty
  • 70c7b1d: 8250607: C2: Filter type in PhiNode::Value() for induction variables of trip-counted integer loops
  • bd8693a: 8256181: Remove Allocation of old generation on alternate memory devices functionality
  • 4df8abc: 8255787: Tag container tests that use cGroups with cgroups keyword

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready label Oct 8, 2020
Copy link

@coleenp coleenp left a comment

just a drive-by comment.

src/hotspot/share/classfile/vmSymbols.hpp Outdated Show resolved Hide resolved
Copy link
Member

@PaulSandoz PaulSandoz left a comment

Reviewed this when updated in panama-foreign, hence the lack of substantial comments for this PR.

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Oct 9, 2020

When re-running all benchmarks, I noted an issue with the BulkOps microbenchmark: calling MemorySegment::mismatch on a small segment (< 8 bytes) was 10x slower than with ByteBuffers. After some investigation, I realized that the issue is caused by the fact that the AbstractMemorySegmentImpl::mismatch method contains inexact var handle calls, where the segment coordinate has type AbstractMemorySegmentImpl instead of the expected MemorySegment, so we take the slow path.

A simple solution is to avoid using var handles directly here, and use the helper functions in MemoryAccess, which widens the type accordingly and produce an exact var handle call.

With this change, perfomance of mismatch on small segment is on par with ByteBuffer.

@@ -75,8 +76,8 @@ public static VarHandle fixUpVarHandle(VarHandle handle) {
return MemoryHandles.filterCoordinates(handle, 0, ADDRESS_FILTER);

This comment has been minimized.

@PaulSandoz

PaulSandoz Oct 9, 2020
Member

The above comment needs updating to refer to MemorySegmentProxy and MemorySegment.

Copy link
Member

@magicus magicus left a comment

Build changes look good.

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Oct 12, 2020

I've just uploaded a biggie update to the foreign memory access support. While doing performance evaluation, we have realized that mixing a multi-level hierarchy (MappedMemorySegment extends MemorySegments) with exact invoke semantics of VarHandle and MethodHandle is not a good match and can lead to great performance degradation for seemingly "correct" code.

While some of this can be attributed to the VarHandle API, or to the fact that the so called "generic" invocation path should not be that slow in case where the parameters are clearly related, it seems smelly that a primitive API such as MemorySegment should give raise to such issues.

We have therefore decided to drop the MappedMemorySegment - this means that there's only one memory segment type users can deal with: MemorySegment - and no chance for mistakes. Of course MappedMemorySegment has been primarily introduces to allow for operations which were previously possible on MappedByteBuffer such as force. To support these use cases, a separate class has been introduced, namely MappedMemorySegments (note the trailing S). This class contains a bunch of static methods which can be used to achieve the desired effects, without polluting the MemorySegment API.

A new method has been added on MemorySegment which returns an optional file descriptor; this might be useful for clients which want to guess whether a segment is in fact a mapped segment, or if they need (e.g. in Windows) the file descriptor to do some other kind of low level op.

I think this approach is more true to the goals and spirit of the Foreign Memory Access API, and it also offers some ways to improve over the existing API: for instance, the only reason why the MemorySegment::spliterator method was a static method was that we needed inference, so that we could return either a Spliterator<MemorySegment> or a Spliterator<MappedMemorySgement>. All of that is gone now, so the method can return to be what it morally always has been: an instance method on MemorySegment.

Updated javadoc:
http://cr.openjdk.java.net/~mcimadamore/8254162_v2/javadoc/jdk/incubator/foreign/package-summary.html

Updated specdiff:
http://cr.openjdk.java.net/~mcimadamore/8254162_v2/specdiff/overview-summary.html

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Oct 29, 2020

I've just uploaded another iteration which addresses some comments from @AlanBateman. Basically, there are some operations on Channel and Socket which take ByteBuffer as arguments, and then, if such buffers are direct, they get the address and pass it down to some native function. This idiom is problematic because there's no way to guarantee that the buffer won't be closed (if obtained from a memory segment) after the address has been obtained. As a stop gap solution, I've introduced checks in DirectBuffer::address method, which is used in around 30 places in the JDK. This method will now throw if (a) the buffer has a shared scope, or (b) if the scope is confined, but already closed. With this extra check, I believe there's no way to misuse the buffer obtained from a segment. We have discussed plans to remove this limitations (which we think will be possible) - but for the time being, it's better to play the conservative card.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 1, 2020

I looked through the changes in this update.

The shared memory segment support looks sound and the mechanism to close a shared memory segment is clever (albeit a bit surprising at first that it does global handshake to look for a frame in a scoped region. Also surprising that close can cause failure at both ends - it took me a while to see that this is pragmatic approach).

The share method specifies NPE if thread == null but there is no thread parameter, is this a cut 'n paste error? Another one in registerCleaner where it should be NPE if the cleaner is null.

I think the javadoc for the close method needs to be a bit clearer on the state of the memory segment when IllegalStateException is thrown. Will it be marked "not alive" when it fails? Does this mean there is a resource leak? I think an apiNote to explain the rational for why close is not idempotent is also needed, or maybe it should be re-visited so that close is a no-op when the memory segment is not alive.

Now that MemorySegment is AutoCloseable then maybe the term "alive" should be replaced with "open" or "closed" and isAlive replaced with isOpen is isClosed.

FileDescriptor can be attraction nuisance and forced reference counting everywhere that it is used. Is it needed? Could an isMapped method work instead?

mapFromPath was in the second preview but I think the method name should be re-examined as it maps a file, the path just locates the file. Naming is subjectives but in this case using "map" or "mapFile" would fit beside the allocateNative methods.

MappedMemorySegments. The force method specifies a write back guarantee but at the same time, the implNote in the class description suggests that the methods might be a no-op. You might want to adjust the wording to avoid any suggestion that force might be a no-op.

The javadoc for copyFrom isn't changed in this update but I notice it specifies IndexOutOfBoundException when the source segment is larger than the receiver, have other exceptions been examined?

I don't have any any comments on MemoryAccess except that it's not immediately clear why there are "Byte" methods that take a ByteOrder. Make sense for the multi-byte types of course.

The updates the java/nio sources look okay but it would be helpful if the really long lines could be chopped down as it's just too hard to do side-by-side reviews when the lines are so long. A minor nit but the changes X-Buffer.java.template mess up the alignment of the parameters to copyMemory/copySwapMemory methods.

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Nov 2, 2020

Now that MemorySegment is AutoCloseable then maybe the term "alive" should be replaced with "open" or "closed" and isAlive replaced with isOpen is isClosed.

While the reason for the method being called "isAlive" are mostly historical (the old Panama pointer API had such a method), I think I still stand behind the current naming scheme. For temporal bounds, I think "isAlive" works better than "isOpened".

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Nov 2, 2020

MappedMemorySegments. The force method specifies a write back guarantee but at the same time, the implNote in the class description suggests that the methods might be a no-op. You might want to adjust the wording to avoid any suggestion that force might be a no-op.

The comment that this operation could be no-op was borrowed from the MappedByteBuffer API; looking at the impl, it seems that you are right that, under no circumstances (unless the segment has length zero) this should be a no-op. How do you suggest I proceed?

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Nov 2, 2020

The javadoc for copyFrom isn't changed in this update but I notice it specifies IndexOutOfBoundException when the source segment is larger than the receiver, have other exceptions been examined?

This exception is consistent with other uses of this exception throughout this API (e.g. when writing a segment out of bounds).

* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation. Oracle designates this
* particular file as subject to the "Classpath" exception as provided

This comment has been minimized.

@mrserb

mrserb Nov 2, 2020
Member

"Classpath exception" could be dropped?

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 4, 2020

I assume the CSR needs to be updated so that it's in sync with the API changes in the latest round.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 8, 2020

I see the xxxByteAtIndex methods that took a ByteOrder have been removed from MemoryAccess. Should the xxxByte and xxxByteAtOffset that take a ByteOrder be removed too?

Remove index-based version of byte getter/setter in MemoryAccess
@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Nov 9, 2020

I see the xxxByteAtIndex methods that took a ByteOrder have been removed from MemoryAccess. Should the xxxByte and xxxByteAtOffset that take a ByteOrder be removed too?

I've addresses this in the latest iteration. Since I was there I also removed getByteAtIndex and getByteAtIndex, since their behavior is identical to that of getByteAtOffset and setByteAtOffset, respectively (in other words, the indexed variants are not really helpful until carrier size > 1 byte).

@mcimadamore
Copy link
Contributor Author

@mcimadamore mcimadamore commented Nov 12, 2020

/integrate

@openjdk openjdk bot closed this Nov 12, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2020

@mcimadamore Since your change was applied there have been 7 commits pushed to the master branch:

  • c6ab0fd: 8255990: Bitmap region of dynamic CDS archive is not unmapped
  • 943acd2: 8256276: Temporarily disable gtest special_flags
  • 19bade0: 8256238: Remove Matcher::pass_original_key_for_aes
  • f7685a4: 8256203: Simplify RegMask::Empty
  • 70c7b1d: 8250607: C2: Filter type in PhiNode::Value() for induction variables of trip-counted integer loops
  • bd8693a: 8256181: Remove Allocation of old generation on alternate memory devices functionality
  • 4df8abc: 8255787: Tag container tests that use cGroups with cgroups keyword

Your commit was automatically rebased without conflicts.

Pushed as commit 3e70aac.

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

openjdk-notifier bot referenced this pull request Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants