Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoC: C++ ASIO #4744

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

PoC: C++ ASIO #4744

wants to merge 1 commit into from

Conversation

tigerw
Copy link
Member

@tigerw tigerw commented May 20, 2020

What do you guys think? It certainly has the potential to make networking much more succinct.

@peterbell10
Copy link
Member

I think ASIO is highly regarded so if we were starting from scratch, ASIO seems like a good choice. However, rewriting the existing networking code using ASIO seems like a lot of work and I'm not sure what we really gain from it. The current networking code has been very stable (not touched is 3-4 years). AFAIK, we also don't have any serious networking bugs or missing features that would justify such a major rewrite.

@tigerw
Copy link
Member Author

tigerw commented May 21, 2020

I'm intending to do small steps, like this one, so definitely no thousand line diffs :)

A migration would get us using

  • interfaces with a good chance of becoming part of the standard
  • no more lengthy cmake configures (esp. on Windows) since it's header-only
  • using scalable I/O completion instead of select() on Windows
  • cleaner code with no more C-style book keeping
  • and best of all a generic threadpool, that we can start migrating the various dedicated-thread-per-world 🗺 components into using

@peterbell10
Copy link
Member

using scalable I/O completion instead of select() on Windows

AFAICT, bufferevent is backed by IO completion ports and not select().

It also looks like ASIO uses the same "iocp" API, judging by the win_iocp file naming:
https://github.com/chriskohlhoff/asio/tree/6f51579783df620267db1f1def799607d5497810/asio/include/asio/detail/impl

and best of all a generic threadpool, that we can start migrating the various dedicated-thread-per-world world_map components into using

Having separate threads for generating, lighting, etc. means the OS scheduler ensures some fairness between them. Imagine you queue 1000 chunks for generating and suddenly the server stops accepting client connections because the authenticator task doesn't get a chance to run.

In a thread pool, you also need to be very careful about blocking code. e.g. the world storage thread wouldn't work as a thread pool task since it's based on blocking file IO.

@tigerw
Copy link
Member Author

tigerw commented May 21, 2020

Libevent vs ASIO

libevent is blocked in a select for me, but asio is in an iocp call

ASIO vs Libevent

Yeah good point with the fairness, certainly we shouldn't post work onto the io_context that's accepting connections. Such a pity there's no cross platform support for async file operations... In any case we're still some ways away from a thread pool, this PR is only for the networking side :)

@tigerw tigerw force-pushed the SeeMake branch 3 times, most recently from 827478d to 46d102c Compare May 23, 2020 13:38
@madmaxoft
Copy link
Member

Could you measure the difference in time between full rebuild of master as-is and with this PR? The problem with header-only libraries is that often they increase compilation times significantly. The Factorio team had to get rid of boost for this reason - it increased their build times by order of tens of minutes.

@tigerw
Copy link
Member Author

tigerw commented May 30, 2020

Sure, hold on a bit.

Base automatically changed from SeeMake to master July 12, 2020 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants