Don't lock the MVar closure on tryReadMVar
ClosedPublic

Authored by dfeuer on Jun 28 2018, 10:07 PM.

Details

Summary

If I understand things correctly, it shouldn't be necessary
to lock the MVar closure on tryReadMVar, since it just
reads one field of the structure and doesn't make any
any modifications. Let's try not doing that.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfeuer created this revision.Jun 28 2018, 10:07 PM
osa1 accepted this revision.Jun 29 2018, 1:12 PM
osa1 added a subscriber: osa1.

This makes sense to me. There's only one read operation here and no need to lock the closure for that.

This revision is now accepted and ready to land.Jun 29 2018, 1:12 PM
This revision was automatically updated to reflect the committed changes.

I should check and see what is the right thing to do with this and other MVar operations on a relaxed memory machine. It is possible that this change has some implications in that direction as tryReadMVar could be reordered differently with the new implementation.

osa1 added a comment.Jun 30 2018, 12:50 AM

Perhaps we should revert this before we forget about this.

I must admit I don't understand these issues with memory models too well, I shouldn't have reviewed this ...

I have vague memories that I thought about this and didn't think it would be safe to remove the lock, but silly me I didn't leave a comment.

Yes, I think we should revert this; it's not at all clear to me that this is correct.

winter added a subscriber: winter.Jun 30 2018, 9:21 PM
This comment was removed by winter.
osa1 added a comment.EditedJul 4 2018, 9:08 AM

Reverted with 8f449955e0417ca7b2d3b324262aa8d1a87ad822 in coordination with @dfeuer