Re: Fwd: kqueue benchmark

Thanks Andi! Johan, could you review and (all being well) apply please? Thanks Ian On Sat, Jan 05, 2013 at 02:16:20PM -0500, Andreas Voellmy wrote:

Hi Ian and Simon,

As you may know, I've been working on an update to GHC's IO manager component that substantially improves the performance and scalability (with cores) of this component. For example, the attached graph shows the performance of a highly simplified web server written in Haskell vs GHC 7.7 HEAD and vs a C web server that was written to do essentially the same thing as my Haskell web server, minus all the Haskell overhead. With my IO manager update, we get about 2x the performance of GHC 7.7 HEAD with 1 core, and over 20x the performance when use lots of cores. We also come very close to the performance of the C web server. (I can give you much more detail on the experiment if you like - just let me know).

Actually, this Haskell web server with the IO manager update allowed me to push linux hard enough to find an elusive bug in the linux kernel that has existed since Linux 2.6! After finding some weird behavior with my Haskell web server, I wrote the C web server (the one I mentioned above) to simulate the Haskell code's use of the kernel, but only using C. After a few weeks of work, the kernel developers found a missing memory barrier and committed a patch to Linux: https://github.com/torvalds/linux/commit/128dd1759d96ad36c379240f8b9463e8acf... .

Bryan O'Sullivan recently reviewed my commits and has suggested that I now ask you to pull changes into GHC. My changes required two minor commits to the main repository and lots of commits to the base library repository. I've posted my work on github:

main repository: https://github.com/AndreasVoellmy/ghc-arv/tree/parallel-iomanager-reimp-2 (last two commits)

and base library repository: https://github.com/AndreasVoellmy/ghc-base-arv/tree/parallel-iomanager-reimp... (all commits following 3fb1aac)

You may not want to review all the commits in my base library repo, but it would be great if you could look at the two commits in the main repo, because they add a couple hooks from the RTS to the IO manager.

Let me know if you need any more clarification or more revisions. I'm not sure exactly what the correct protocol is for getting these changes in, so let me know if I need to do something like open a ticket, etc.

Cheers, Andi

---------- Forwarded message ---------- From: Bryan O'Sullivan Date: Fri, Jan 4, 2013 at 11:43 AM Subject: Re: kqueue benchmark To: Andreas Voellmy Cc: Johan Tibell , Kazu Yamamoto

Thanks, Andi. At this point, I think you should just go ahead and ask Ian or Simon to pull your work into the relevant ghc trees. It's looking great.

On Jan 4, 2013, at 8:39, Andreas Voellmy wrote:

Bryan: thanks for the detailed review and comments! I've gone through all of your comments and for almost all of them I made the change you suggested (I.e. fixing the mistake in calling eventfd_write, improving comments, reducing use of conditional compiles, etc.). The only exceptions were (1) one minor tidying up change you suggested that became obsolete due to a later change and (2) the comment on reducing the number of capabilities at runtime. For this one, you asked whether the RTS supports it. It does support it. You also mentioned that it seems a bit complex. I agree, but I don't think it's too bad, so I've just left it alone.

I also made a couple commits to address things with closeFdWith, closeFd, and closeFd_ that I mentioned in my last email.

I pushed a new branch here: https://github.com/AndreasVoellmy/ghc-base-arv/tree/parallel-iomanager-reimp...

It has been rebased against the latest HEAD and includes the following new commits:

These address Bryan's comments: f33364a Fix wrong type in FFI call to eventfd_write in GHC.Event.Control. cd7e381 Avoid conditional compilation in GHC.Event.Manager. 7af055c Undo recent change to the type of GHC.Event.Thread.getSystemEventManager and update the commentary on this function

These are fixes to closeFdWith, closeFd, and closeFd_: c925187 closeFdWith invokes callbacks only after the fd is closed. bb015fe Tidy up GHC.Event.Thread.closeFdWith. 879b0f1 closeFdWith closes fd after unregistering the fd with the backend. b508e1a Update closeFd_ to avoid unnecessary backend modifications. 44ed970 Change GHC.Event.Manager.closeFd to unregister interest in the file with the backend. 3622106 Avoid use of backend modifyFdOnce in Poll backend in unregisterFd_. de608e5 Added missing wakeup in GHC.Event.Manager.closeFd_. 639ee76 Improve comment on GHC.Event.Manager.closeFd_.

On Mon, Dec 31, 2012 at 6:18 PM, Bryan O'Sullivan wrote:

...

Last code review of 2012 finished :-)

I've commented on all your commits on github. It's a very well put together patchset, and was easy to review. Thanks for going to the extra effort to make the patches small enough to be reviewable.

There was only one obviously wrong commit, which I flagged. Apart from that, the only substantial comment I had was about the increased number of #ifdefs. I believe that many of those could be done away with, which would make the code both cleaner and more statically checkable on more platforms - I'll see if I can find time to confirm that theory.

I'm sure you've seen that people are justifiedly rather excited about your work: https://twitter.com/bos31337/status/284701554458640384

Great work! B.

On Fri, Dec 28, 2012 at 3:26 AM, Andreas Voellmy < andreas.voellmy@gmail.com> wrote:

...

The changes were almost entirely in the base package. You can find them here:

https://github.com/AndreasVoellmy/ghc-base-arv/tree/parallel-iomanager-reimp...

I also had to add a couple of RTS hooks, so there are also two commits in the main repo, which you can see here:

https://github.com/AndreasVoellmy/ghc-arv/tree/parallel-iomanager-reimp-2