search

adinapoli / threads-supervisor

Simple, IO-based Haskell library for Erlang-inspired thread supervisors
MIT License
29 stars 4 forks source link

clean up event handling #14

Closed srijs closed 8 years ago

srijs commented 8 years ago

In preparation for OneForAll, this cleans up the event handling structure a bit.

adinapoli commented 8 years ago

Hey @srijs , thanks for this PR!

As it seems (and I'm very happy about that!) we will be working together for a while, and considering I can review this only when I'm a zombie (either before work in the morning or late in the evening), may I check in that, for each PR we send we ran the testsuite locally with green lights? (Travis CI should run it for us, but I don't trust Travis too much right now as I have hacked together a .travis.yml a while ago to support stack and I definitely want to overhaul it). Also, we should probably try to improve the testsuite. Although quickcheck-generated, we should inspect the hpc coverage and test the uncovered areas!

Sorry, I know that this sounds really pedagogical and it's not my intention! It's rather trying to exploit machines in a way they can make up for my sleepy brains when I review :wink: More jokes aside, it's very tough to review PRs which deals with concurrency and async exceptions, as disaster could strike at any minute. Therefore I'm much more liberal in merging as I know that there are very little chances I will spot a regression only by reading a diff.

Aaaanyway, sorry for the long digression! :+1: for me, feel free to merge it yourself after you feel satisfied and if you get green lights on your machine. Thank you!

srijs commented 8 years ago

Thanks, @adinapoli.

Definitely +1 on more test coverage. That's one of my biggest concerns as well when working on this code. I will try and see if I can come up with a few more tests so we can gain more confidence in each others changes.

In my opinion, a big part of that as well is how the code is structured. Right now, most of the functions run in the IO monad, which tbh makes me a bit nervous as far as test coverage goes :) Since the handleEvents loop (probably the most critical component to test) runs serially, maybe we should invest some effort into making it side-effect free, for example using a free monad. That means we could test it in pure code.

The tests are green on my machine, but since this is a "clean-up" PR, I might just go ahead and encode our side-effects in a free monad while I'm at it (and write more tests of course). Let me know what you think!

adinapoli commented 8 years ago

I definitely see your point, but as Free monads are performance taxing and the aim of this library is to be "as fast as forkIO", my priority would be first to make it work the way we want, and then to purify it with free monads, provided we can maintain a decent level of performance!

So, in a nutshell, would you mind for now keeping the free monad in the back of your brain and we will revisit when we are ready? :wink: Thanks!

srijs commented 8 years ago

It's true that construction-based free monads (so your typical ones) are going to be slow, because they are allocating a lot of constructors which are then immediately torn down.

However, as long as we are only exposing an interface based on IO, I think there are ways to not lose any performance. What I have in mind is something along the lines of the Finally Tagless approach, or the Van Laarhoven encoding.

In fact, it wouldn't look much different than the abstraction you introduced via the QueueLike class. In addition to that, when the exported symbols are purely based on IO, I believe we can instruct the compiler to properly inline, and thus eliminate the abstraction through monomorphisation, leading to the same performance as without it.

Let me merge this PR first, then I'll do a second one to try and convince you that it's possible ;)

adinapoli commented 8 years ago

That's good to hear, thanks! (I heard of those 2 approaches but never used them in practice, as all the times I needed Free monads were for internal DLSs, which I did not need them to be performance critical).

I would be :+1: for that, but my only pet peeve would be to not cause any feature creep and keep the free monad story separated from this PR. You can open a new ticket and assign it to you, if you want to have fun with it. Sorry for being fussy! :wink:

adinapoli commented 8 years ago

:tada: :fireworks:

adinapoli commented 8 years ago

@srijs On the Free monad topic (I know, I should open a separate issue!) I just watched this great talk from Raichoo which might be indeed food for thought:

https://vimeo.com/146374255

This is the paper referenced in the talk, which exploits the notion of Codensity:

http://www.janis-voigtlaender.eu/papers/AsymptoticImprovementOfComputationsOverFreeMonads.pdf