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

Add async threads #2219

Open
wants to merge 8 commits into
base: master
from

Conversation

@flaviojs
Copy link
Contributor

commented Jul 17, 2018

This PR introduces a way to split loading/processing stuff and GUI stuff.

An example split is done in the WaitingForGameStart state of the human client.
The data is loaded in an async thread. When done it adds the UI updates to the GUI work queue, which is executed before the next frame is rendered.
As a consequence, when you continue a game from the intro menu, instead of being frozen you will be able to move the mouse during the initial load (before the PlayingTurn state).

Topic: http://www.freeorion.org/forum/viewtopic.php?f=9&t=11009

flaviojs added 3 commits Jul 17, 2018
The code is stored in a thread-safe work queue.
It is executed before the next frame is rendered.
@Vezzra Vezzra added this to the post 0.4.8 milestone Jul 17, 2018
@Dilvish-fo

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Particularly in light of that last commit that was just added, it seems like this should probably be labeled as "testing requested" and/or "work in progress"

@flaviojs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

This should be "work in progress" until someone decides what to do with unhandled exceptions in async threads, which is not my decision to make. For now I'm just adding a line to ErrorLogger().

As for the last commit. I added is_loading code when I was reverting changes to trace messages before pushing this PR. I though I had tested it at the time but it seems like I only tried compiling... won't happen again, sorry. :(

//! Update UI in the GUI thread.
void ProcessUI(bool is_new_game, const SaveGameUIData& ui_data);

volatile bool is_loading = false; //!< true from GameStart to DoneLoading

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 18, 2018

Member

Are you sure you know what you're doing with volatile?

http://cxx.isvolatileusefulwiththreads.com

This comment has been minimized.

Copy link
@flaviojs

flaviojs Jul 19, 2018

Author Contributor

Atomic access is not needed here, so volatile is enough. Do you prefer std::atomic?

It is written in one thread and read in another, but the access is sequential (not concurrent). I just need to make sure it is not reading a cached value, therefore volatile is enough.

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

If your answer is 'yes', that's fine.

@@ -782,29 +783,61 @@ WaitingForGameStart::WaitingForGameStart(my_context ctx) :
}

WaitingForGameStart::~WaitingForGameStart()
{ TraceLogger(FSM) << "(HumanClientFSM) ~WaitingForGameStart"; }
{

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 18, 2018

Member

{ to previous line

@@ -1044,6 +1044,29 @@ std::vector<std::shared_ptr<GG::Texture>> ClientUI::GetPrefixedTextures(const bo
return prefixed_textures_and_dist.first;
}

bool ClientUI::PushWork(std::function<void()> work)
{

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

same adjustment to { in various other non-template places with a single line function name and parameters

/** Add work for the GUI thread. */
bool PushWork(std::function<void()> work);

/** Get and remove work for the GUI thread. */

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

What does it mean to "remove work for the GUI thread" ?

boost::statechart::result react(const GameStart& msg);

CLIENT_ACCESSOR

//! Load data in an asyc thread.

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

what data? explain what this does / how / why.
should these functions be private?

@@ -36,6 +37,9 @@ struct TurnEnded : boost::statechart::event<TurnEnded> {};
// Posted to advance the turn, including when auto-advancing the first turn
struct AdvanceTurn : boost::statechart::event<AdvanceTurn> {};

// Indicates that everything has been loaded.

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

what does "loaded" mean?

@@ -815,6 +848,13 @@ boost::statechart::result WaitingForGameStart::react(const GameStart& msg) {
GetGameRules().SetFromStrings(Client().GetGalaxySetupData().GetGameRules());

bool is_new_game = !(loaded_game_data && ui_data_available);
Client().GetClientUI().PushWork(

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

not clear about what's going on / why. ProcessData is called within launch_async but then it calls the PushWork function... why in that order?

@@ -826,7 +866,7 @@ boost::statechart::result WaitingForGameStart::react(const GameStart& msg) {

Client().GetClientUI().GetPlayerListWnd()->Refresh();

return transit<PlayingTurn>();
context<HumanClientFSM>().process_event(DoneLoading());

This comment has been minimized.

Copy link
@geoffthemedio

geoffthemedio Jul 19, 2018

Member

why does this function generate the DoneLoading event and not the caller? It and ProcessData and ProcessUI don't seem to be used elsewhere.

@flaviojs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@geoffthemedio
I guess an explanation on how this works is in order.

Currently the main thread is used as GUI thread and processing thread, causing visible freezes every time processing takes a bit longer than an instant (for me it drops to 0/1 FPS quite often).

In OpenGL, almost every interaction must happen in the thread that created the OpenGL context. It is possible to create additional contexts but that would only complicate things.

If I want to separate processing from the GUI thread, then the processing code must have some way to tell the GUI thread that it can execute the UI code that depends on the processed data.

In this approach the processing code can add work (arbitrary code as a std::function<void()>) to a queue.
ClientUI is used as a singleton and is accessible to all UI code of the human client, so it is a suitable place to hold the FIFO work queue for the GUI thread.
HumanClientApp::HandleSystemEvents is called by the GUI thread just before rendering a frame, so it is a suitable place to execute the work that was added to the queue.

In short, GUI work is arbitrary code to be executed before rendering a frame, and any thread can add GUI work. Some renaming might be needed to make it clearer, but I can't think of better names...


Now then, let's explain the split in WaitingForGameStart...

When the GameStart message is received, HumanClientApp::HandleSystemEvents will call HumanClientApp::HandleMessage, which calls m_fsm->process_event(GameStart(msg));, which in turn calls WaitingForGameStart::react(const GameStart& msg).

The code here modifies Universe-related data and then updates the UI.

What I did was split that into two functions, WaitingForGameStart::ProcessData and WaitingForGameStart::ProcessUI, and make the first one execute in an async thread.

This is the new sequence:

  1. GUI thread executes WaitingForGameStart::react(const GameStart& msg) and launches an async
  2. async thread executes WaitingForGameStart::ProcessData while the GUI thread continues executing normally (processing keyboard events, processing mouse events, processing network messages, and rendering frames)
  3. async is done and tells the GUI thread to execute WaitingForGameStart::ProcessUI (by pushing work to the queue)
  4. GUI thread pops work from the queue and executes WaitingForGameStart::ProcessUI (before rendering the frame)

WaitingForGameStart::ProcessUI is called from HumanClientApp::HandleSystemEvents so it can't transit state (FSM isn't being processed), and since the FSM is only processed when a network message arrives I can't just post a DoneLoading event either (would be processed when a network message arrives).

Ideally the FSM would run in it's own thread, allowing you to post events from anywhere, but that would require splitting all UI code from the react functions of the FSM and that is not what this PR is about.

Since WaitingForGameStart::ProcessUI is already executing in the thread that processes the FSM, it is sufficient to call process_event(DoneLoading()) on the FSM.

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Too busy at the moment to go through all you wrote, but I particularly wanted any non-clears bits of that explained in the code with comments.

flaviojs added 3 commits Jul 29, 2018
They are in a struct but they don't need to be public.
@flaviojs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

@geoffthemedio I tried to answer all your questions with comments in the code.

It would be nice if you guys documented the preferred code style.
The wiki says to try to match surrounding code, but in ClientUI there is a mixture of styles. (example: around PushWork/PopWork there are different function bracket styles; on a new line; next to the function declaration; open and close on the same line)

@geoffthemedio

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

I've tested this now, in comparison with master, with a 650 star galaxy and about 10 AIs, medium or high on most settings. I don't see much difference... There's a less-than-half second pause for the mid-turn update, and a slightly longer pause for the main turn update, during which the UI seems to hang. Seems quite similar for both. Do you have a better test case or expect to see a more substantial difference with this pull request applied?

@Vezzra Vezzra removed this from the Next Release milestone Sep 22, 2019
@flaviojs

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

I'm not gonna be around to continue this so maybe it should be closed?

I hate seeing software freeze. From a UI perspective, not giving feedback is bad.
The aim of this was not to improve speed, but to allow the game to give feedback to the user, which is mouse movement in this case.

@Vezzra

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

@geoffthemedio, so do I understand correctly, even if @flaviojs isn't going to work on this anymore, you want that to keep it open and continue working on it/wait for someone to pick it up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.