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

Fix concurrent MemoryScope.close() or MemoryScope.dup() race #160

Conversation

@plevart
Copy link
Contributor

@plevart plevart commented May 11, 2020

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.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/160/head:pull/160
$ git checkout pull/160

@plevart plevart changed the base branch from foreign-jextract to foreign-memaccess May 11, 2020
@plevart
Copy link
Contributor Author

@plevart plevart commented May 11, 2020

The foreign tests pass and ParallelSum benchmark doesn't show regressions:

before fix:

Benchmark                                         Mode  Cnt     Score     Error  Units
ParallelSum.segment_parallel                      avgt   30    42.298 ±   0.139  ms/op
ParallelSum.segment_parallel_bulk                 avgt   30    38.268 ±   0.318  ms/op
ParallelSum.segment_serial                        avgt   30   109.087 ±   0.618  ms/op
ParallelSum.segment_stream_findany_parallel       avgt   30  1721.486 ± 306.951  ms/op
ParallelSum.segment_stream_findany_parallel_bulk  avgt   30    32.263 ±   1.572  ms/op
ParallelSum.segment_stream_findany_serial         avgt   30  5090.839 ±  19.335  ms/op
ParallelSum.segment_stream_findany_serial_bulk    avgt   30   105.765 ±   1.527  ms/op
ParallelSum.segment_stream_parallel               avgt   30    38.995 ±   0.236  ms/op
ParallelSum.segment_stream_parallel_bulk          avgt   30    38.146 ±   0.225  ms/op
ParallelSum.unsafe_parallel                       avgt   30     6.476 ±   0.226  ms/op
ParallelSum.unsafe_serial                         avgt   30   107.892 ±   0.882  ms/op

after fix:

Benchmark                                         Mode  Cnt     Score     Error  Units
ParallelSum.segment_parallel                      avgt   30    42.195 ±   0.094  ms/op
ParallelSum.segment_parallel_bulk                 avgt   30    38.216 ±   0.269  ms/op
ParallelSum.segment_serial                        avgt   30    98.562 ±   9.357  ms/op
ParallelSum.segment_stream_findany_parallel       avgt   30  1532.052 ± 325.223  ms/op
ParallelSum.segment_stream_findany_parallel_bulk  avgt   30    32.167 ±   1.664  ms/op
ParallelSum.segment_stream_findany_serial         avgt   30  5443.200 ± 370.581  ms/op
ParallelSum.segment_stream_findany_serial_bulk    avgt   30   104.543 ±   0.924  ms/op
ParallelSum.segment_stream_parallel               avgt   30    39.010 ±   0.281  ms/op
ParallelSum.segment_stream_parallel_bulk          avgt   30    38.161 ±   0.314  ms/op
ParallelSum.unsafe_parallel                       avgt   30     6.386 ±   0.250  ms/op
ParallelSum.unsafe_serial                         avgt   30    99.642 ±   8.711  ms/op
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 12, 2020

👋 Welcome back plevart! A progress list of the required criteria for merging this PR into foreign-memaccess will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 12, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 12, 2020

Webrevs

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

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?

@plevart
Copy link
Contributor Author

@plevart plevart commented May 13, 2020

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 13, 2020

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

Cool! Then I suggest you maybe close this PR and I will submit another with the new impl, if that's ok?

@plevart
Copy link
Contributor Author

@plevart plevart commented May 13, 2020

Closing this PR in favor of Maurizio's implementation.

@plevart plevart closed this May 13, 2020
@plevart plevart deleted the plevart:foreign-memaccess-close-race-fix branch May 15, 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

2 participants
You can’t perform that action at this time.