[foreign-memaccess] RFR: Fix concurrent MemoryScope.close() or MemoryScope.dup() race

[Peter Levart]

On Tue, 12 May 2020 13:54:56 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> In case a MemorySegment is not confined to an "owner" thread, it is possible to call close() or
>> withOwnerThread(newThread) on it from multiple threads concurrently and such calls are not screened for thread
>> confinement but just forwarded to MemoryScope.close() and .dup(). It is therefore vital that close() and dup() are made
>> atomic so that cleanupAction is executed just once.
>
> I've been playing around a bit and discussed this offline with @JornVernee too. A realization I had some time ago is
> that what we're trying to do here is a read/write lock, where multiple acquire can occur at the same time, but where
> close of the root should be exclusive. I think all the machinery around CLOSING is essentially due to the code trying
> to mimic that pattern.  I've put together this code:
> https://gist.github.com/mcimadamore/54fecd888bcdc8fcf8aefaa51d4cbcbd
> Which, I think, simplifies over the status quo, and also adds back the atomicity seeked in the proposed patch. Also,
> since now there some guarantee that no acquire can take place while a close is also taking place (and pending acquires
> will be invalidated - using the optimistic read logic), then we can just use a single long adder instead of two.  The
> state is also simplified, since we just need a boolean flag.
> This seems to provide same numbers as what we have now, but IMHO the code is bit easier to follow, as there are less
> moving parts. What do you think?

On 5/12/20 3:55 PM, Maurizio Cimadamore wrote:
>
> *@mcimadamore* commented on this pull request.
>
> I've been playing around a bit and discussed this offline with
> @JornVernee <https://github.com/JornVernee> too. A realization I had
> some time ago is that what we're trying to do here is a read/write
> lock, where multiple acquire can occur at the same time, but where
> close of the root should be exclusive. I think all the machinery
> around CLOSING is essentially due to the code trying to mimic that
> pattern.
>
> I've put together this code:
> https://gist.github.com/mcimadamore/54fecd888bcdc8fcf8aefaa51d4cbcbd
>
> Which, I think, simplifies over the status quo, and also adds back the
> atomicity seeked in the proposed patch. Also, since now there some
> guarantee that no acquire can take place while a close is also taking
> place (and pending acquires will be invalidated - using the optimistic
> read logic), then we can just use a single long adder instead of two.
>
> The state is also simplified, since we just need a boolean flag.
>
> This seems to provide same numbers as what we have now, but IMHO the
> code is bit easier to follow, as there are less moving parts. What do
> you think?
>

I think this is correct implementation. And perhaps easier to follow
too. Nice use of StampedLock which hides the complexity of read-write
locking under the easy to use API. And the scalable counter state is
reduced by half without reducing scalability too, which is also an
improvement - a particular thread typically stabilizes to always update
the same cell in the LongAdder so there should be little to no
contention even with one counter.

I think this should replace current version. I'm all for it. Good work.


Regards, Peter


> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/openjdk/panama-foreign/pull/160#pullrequestreview-410066310>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAMBYGSBAIYTAMRMR6J5XSTRRFIL7ANCNFSM4M6JYTMQ>.
>

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

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