Opened 20 months ago

Closed 15 months ago

Last modified 3 months ago

#7216 closed feature request (fixed)

Compositional blocking on file descriptors

Reported by: AndreasVoellmy Owned by: igloo
Priority: normal Milestone: 7.8.1
Component: libraries/base Version: 7.4.2
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

The GHC.Event.Thread module provides threadWaitRead, threadWaitWrite :: Fd -> IO () calls that block until a particular file descriptor is ready for reading or writing. With this API a single thread cannot wait on multiple file descriptors or a file descriptor and some other condition (e.g. an MVar, STM transaction, etc). One could work around this by creating extra producer threads that each monitor one file descriptor and then multiplex messages from those threads onto a single channel (or mvar, etc) and then have a consumer thread that waits on this single multiplexed channel. Unfortunately, this extra indirection imposes a modest performance penalty in the form of longer wait times to service events due to having to switch between multiple threads to handle a single ready file.

I propose to provide analogous functions in the STM monad threadWaitReadSTM,threadWaitWriteSTM :: Fd -> STM (). These will allow a single thread to wait on multiple files or both files and other conditions. This eliminates the switching between threads to service a request.

Attachments (6)

stm-fd-wait.patch (2.2 KB) - added by AndreasVoellmy 20 months ago.
Patch against 2921e94a3546d3c431051271f505307d2f13b208
threadWaitSTM.patch (5.4 KB) - added by AndreasVoellmy 18 months ago.
Return an unregistration function with the threadWait*STM functions; added some haddock comments.
expose-threadWaitSTM-on-master.patch (4.9 KB) - added by AndreasVoellmy 16 months ago.
expose-threadWaitSTM-on-master.2.patch (6.3 KB) - added by AndreasVoellmy 16 months ago.
expose-threadWaitSTM-on-master.3.patch (6.3 KB) - added by AndreasVoellmy 16 months ago.
expose-threadWaitSTM-on-master.4.patch (6.1 KB) - added by AndreasVoellmy 16 months ago.

Download all attachments as: .zip

Change History (23)

Changed 20 months ago by AndreasVoellmy

comment:1 Changed 20 months ago by AndreasVoellmy

  • Status changed from new to patch

comment:2 Changed 20 months ago by simonmar

  • Difficulty set to Unknown

The INLINEs are probably unnecessary, but otherwise the patch looks fine to me. Johan or Bryan should give it a once-over too.

comment:3 Changed 18 months ago by tibbe

Looks good to me.

comment:4 Changed 18 months ago by igloo

  • Milestone set to 7.8.1

This patch warns that the return values of registerFd and unregisterFd_ are ignored. Is that correct?

comment:5 Changed 18 months ago by AndreasVoellmy

threadWaitSTM basically follows the template threadWait. threadWait also ignores the result of applications of unregisterFd_, so on this point it is no different than the code that is already in that module.

Regarding the result of registerFd: threadWait uses the result of registerFd in only one place: it creates an empty MVar, registers a callback that fills it and waits on the MVar handling any exceptions thrown to the thread (it unregisters the callback on exception); unregistering is the only place the result of registerFd is used.

threadWaitSTM does not block on an MVar (or anything else, that is
the point of it), so it cannot use the result of registerFd in the
same way. To accomplish the same thing on an exception (i.e. unregistering the callback) we should probably return the registration key in the result of threadWaitSTM so that the user can handle and unregister. This would require also re-exporting the unregistration function in GHC.Event.Thread, since it is currently exported from the non-exposed GHC.Event.Manager module.

On the other hand, I don't really see why unregistering is
necessary for correctness. It seems to me like unregistering is an optimization, since
it keeps the callback table smaller. If this occurs and the registered file is ever ready, then the
callback will fire and be unregistered. If the registered file is
never ready, then the file will never be removed from the callback
table, but this could happen with a file anyway even when no error occurs.

Changed 18 months ago by AndreasVoellmy

Return an unregistration function with the threadWait*STM functions; added some haddock comments.

comment:6 Changed 18 months ago by AndreasVoellmy

The last patch addresses the previous comment by igloo. I made the threadWaitSTM return an IO action that unregisters interest in the file descriptor. By returning an IO () value we avoid having to export the FdKey type and unregsterFd functions.

I also added some haddock comments to threadWaitReadSTM and threadWaitWriteSTM.

comment:7 Changed 17 months ago by igloo

  • Resolution set to fixed
  • Status changed from patch to closed

Applied, thanks!

comment:8 Changed 16 months ago by AndreasVoellmy

The previous patches did not actually expose the new functions in any public module. The attached patch exports these functions in Control.Concurrent, which is where the threadWaitRead and threadWaitWrite functions are exported.

Changed 16 months ago by AndreasVoellmy

comment:9 Changed 16 months ago by simonpj

  • Resolution fixed deleted
  • Status changed from closed to new

comment:10 Changed 16 months ago by simonpj

  • Owner set to igloo

comment:11 Changed 16 months ago by simonpj

  • Status changed from new to patch

comment:12 Changed 16 months ago by igloo

I'm a bit confused. Do these functions work on Windows? In Control.Concurrent they just raise an exception on Windows, but it looks like in GHC.Conc.IO they are defined on Windows.

comment:13 Changed 16 months ago by AndreasVoellmy

I think the confusion also exists for the threadWaitRead and threadWaitWrite functions. They are defined in GHC.Conc.IO even for Windows, but then they seem to be completely ignored in Control.Concurrent and the functions of the same name there are defined for Windows only in the threaded RTS (or for stdin in non-threaded RTS).

comment:14 Changed 16 months ago by AndreasVoellmy

I made a new patch that implements the threadWaitReadSTM and threadWaitWriteSTM functions for Windows with the threaded RTS.

Changed 16 months ago by AndreasVoellmy

Changed 16 months ago by AndreasVoellmy

Changed 16 months ago by AndreasVoellmy

comment:15 Changed 16 months ago by AndreasVoellmy

Sorry for all the patches. You can ignore all, but the last. expose-threadWaitSTM-on-master.2.patch which expose-threadWaitSTM-on-master.3.patch fixed. expose-threadWaitSTM-on-master.4.patch is a minor simplification.

comment:16 Changed 15 months ago by igloo

  • Resolution set to fixed
  • Status changed from patch to closed

Applied, thanks!

comment:17 Changed 3 months ago by twhitehead

It seems a bit strange that the last patch re-implemented different versions of threadWait{Read,Write}STM in Control.Concurrent for Windows (mingw32_HOST_OS) despite the GHC.Conc versions covering both Windows and non-Windows. Why not just

Control.Concurrent.threadWaitReadSTM :: Fd -> IO (STM (), IO ())
Control.Concurrent.threadWaitReadSTM = GHC.Conc.threadWaitReadSTM

Control.Concurrent.threadWaitWriteSTM :: Fd -> IO (STM (), IO ())
Control.Concurrent.threadWaitWriteSTM = GHC.Conc.threadWaitWriteSTM

If the issue is the GHC.Conc versions for Windows are deficient (e.g., they don't handle [pass along] threadWait{Read,Write} exceptions), shouldn't they be fixed instead?

Note: See TracTickets for help on using tickets.