Compactor icon indicating copy to clipboard operation
Compactor copied to clipboard

WIP: Use select over channels for run loops, rather than polling

Open Dr-Emann opened this issue 3 years ago • 1 comments

I wanted to go further in removing polling, I turned {scan/compress/uncompress}_loop into loops over selecting on channels.

The scan loop was simple enough to use the select macro: It just waits for messages from the gui and the result channel, and wakes up every so often to write status.

The compress/uncompress loops were a bit more complex, and required using the Select struct manually (mainly because we needed to optionally try to send something, and we wanted to do work once we know we can send in order to build the thing we're sending). The loops are pretty complicated, but I'm not sure if they're any more or less complicated than they already were. (like the comment says: Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl fhtagn.).

Before, each send was immediately followed by a receive, so we just used the same FileInfo we just sent when we received a response. Because of the channel sizes, we could just store the last sent info, and use that. However, it turns out that this strategy makes it pretty easy to mulithread, by just spinning up multiple background threads with the same input/output channels, so instead I used a hashmap of file infos which are still outstanding.

There are some differences in behavior I think should be fixed before this PR is finished, but I wanted to get this out to see if I'm on an okay path, and if you had any comments:

  • Because displaying summary info is de-coupled from send/receiving, when working on a large file, the summary display is pretty inaccurate: the large file has been removed from the summary: It won't be re-added until we receive the result of compressing/uncompressing the file.
    • I'm not sure if I want to change the FolderInfo to in some way avoid actually removing the files to be processed, or go back to only reporting status after receiving a file from processing. I'm leaning toward the second.
  • When pausing, the old code would display "Pausing after {path}", then wait for the result from the compression/decompression, and only then display "Paused". The new code just jumps immediately to "Paused"
    • I believe this can be done by keeping the receive channel active when paused, and on receive, check if we are paused, and if the map of outstanding file infos is empty, displaying "Paused".

Dr-Emann avatar Dec 24 '20 17:12 Dr-Emann

Impressed to see someone actually diving into this mess, thanks! I like the more thoughtful use of crossbeam and select - had good experiences with that sort of thing in my recent messing with Tokio.

Generally I've considered this code disposable - I pretty much rushed it to get a working app, with the expectation that I'd be dumping it wholesale at a later date when I had a better idea of what I was doing. Ideally that wouldn't be far off, because quite a lot's riding on redoing it, e.g:

  • Multithreading (maybe not that much of a blocker, but would like to make the UI more suitable for it)
  • Optional analysis, it doesn't really serve much purpose beyond making sure the numbers mostly go down
  • Multiple folders/job queue
  • Additional job types, like recompression and dry runs
  • Proper error reporting
  • CLI

Certainly don't want to be making it more complex to add on any of these.

If you think you can resolve the issues with this PR easily enough I'm not going to discourage you - I'm sure people would appreciate earlier access to multithreaded operation - but I can't guarantee this code will last long.

Freaky avatar Dec 29 '20 09:12 Freaky

Closing this on its two-year anniversary. The 0.11 rewrite is proceeding well and completely obsoletes my irredeemable loops-from-hell.

Thanks for your efforts!

Freaky avatar Dec 24 '22 14:12 Freaky