RFR: 8287809: Revisit implementation of memory session [v2]

[Jorn Vernee]

On Tue, 7 Jun 2022 13:00:37 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This is a cleanup of the memory session implementation. The main new concept is that `MemorySessionImpl` is split into two parts: there is an implementation of memory session and then there is a state abstraction (`MemorySessionImpl.State`). This allows to share the state across multiple session views, in a more clean way. The big consequence of this change is that the routines on `ScopedMemoryAccess` now have to be defined in terms of the state abstraction (but the changes are mostly mechanical).
>> 
>> I have consolidated the implementation quite a bit, by removing all the duplicated logic for issuing similar-looking exceptions. I have also addressed an issue with `checkValidState` throwing a _new_ `WrongThreadException` instead of using a singleton (which is what the logic for closing down shared session requires, to avoid stack walks that are too deep).
>> 
>> `MemorySession.State::checkValidState` is now fully monomorphic; when looking at benchmarks, this seems to be the best solution in order to make things fast. Specializing implmentations to remove few plain checks does not buy enough, and always has the risk of adding profile pollution.
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits:
> 
>  - Merge branch 'master' into cleanup_memory_session_impl_state
>  - Add null check on Buffer::checkState
>  - Add docs
>    Simplify liveness check in Buffer
>    Drop redundant import in DirectBuffer
>  - Simplify checkValidState
>  - Add fastpath for implicit session state
>  - Merge branch 'master' into cleanup_memory_session_impl_keep_list
>  - Fix asNonCloseable to return self
>    Improve direct buffer code with isImplicit predicate
>  - Drop MemorySession interface type from AbstractMemorySessionImpl
>  - Simplify code by removing intermediate getUnsafeBase/getUnsafeOffset methods
>  - Simplify readOnly check
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/8d28734e...5b8f7246

I think this looks good overall, but please try to limit the accessibility of  the methods in `State`.

src/java.base/share/classes/jdk/internal/foreign/ConfinedSessionState.java line 40:

> 38:  * operations triggered by threads other than the owner thread, which we support.
> 39:  */
> 40: public final class ConfinedSessionState extends MemorySessionImpl.State {

Same here. I don't see why this is made public now.

src/java.base/share/classes/jdk/internal/foreign/SharedSessionState.java line 46:

> 44:  * checking the liveness bit upon access can be performed in plain mode, as in the confined case.
> 45:  */
> 46: public class SharedSessionState extends MemorySessionImpl.State {

Unclear to me why this is now public. It still seems to be only accessed from with the `jdk.internal.foreign` package?

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

Marked as reviewed by jvernee (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9017