The Wayback Machine - https://web.archive.org/web/20200928031846/https://github.com/cuberite/cuberite/pull/4744
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

Open
wants to merge 1 commit into
base: master
from
Open

PoC: C++ ASIO #4744

wants to merge 1 commit into from

Conversation

@tigerw
Copy link
Member

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

peterbell10 commented May 21, 2020

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

peterbell10 commented May 21, 2020

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 May 21, 2020
@madmaxoft
Copy link
Member

madmaxoft commented May 29, 2020

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.

@tigerw tigerw force-pushed the SeeMake branch from 3ebe1c9 to 6ff4714 Jul 6, 2020
Base automatically changed from SeeMake to master Jul 12, 2020
@tigerw tigerw force-pushed the SeeBackup branch from 73f60e8 to fc07583 Jul 20, 2020
@tigerw tigerw force-pushed the SeeBackup branch from fc07583 to 072fdf3 Jul 26, 2020
@tigerw tigerw mentioned this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.