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

[no squash] Fix two common causes of "app isn't responding" #14512

Merged
merged 2 commits into from Apr 5, 2024

Conversation

grorp
Copy link
Member

@grorp grorp commented Apr 1, 2024

  • Server startup and shutdown block the main thread.
    -> Solution: Server startup and shutdown have been moved to separate threads

  • Loading of cached media blocks the main thread because all cached media is loaded at once.
    -> Solution: Loading of cached media is now done in chunks.
    (Loading of non-cached media is already unproblematic.)

See #14440

To do

This PR is a Ready for Review.

Note: I have only tested this on Linux so far. It still needs to be tested on Android.

How to test

Start a singleplayer session with a heavy game (e.g. MeseCraft or MineClone 2). Verify that the main thread is not blocked in the "Creating server..." and "Media..." phases, e.g. by resizing the window, pressing ESC, trying to close the window, ...

Verify that the main thread is not blocked in the "Shutting down..." phase

Verify that loading of cached media isn't noticeably slower. (Obviously it'll be a bit slower since we're now drawing frames / handling events in-between media loading, but it shouldn't be noticeably slower.)
↑ Please actually test this. ↑

@grorp
Copy link
Member Author

grorp commented Apr 2, 2024

Loading of cached media is now done in chunks.

Even though I've already set a rather large chunk size, loading of cached media is still about 100ms slower than before on the Capture the Flag server, and I haven't quite figured out where that time is being spent.

EDIT: This should be fixed by the latest commits.

src/client/game.cpp Outdated Show resolved Hide resolved
u64 cur_time = porting::getTimeMs();
u64 dtime = porting::getDeltaMs(last_time, cur_time);
if (dtime >= chunk_time_ms) {
client->drawLoadScreen(loading_text, dtime / 1000.0f, 30);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulling the load screen drawing into here feels like an abstraction violation. But I have an idea to improve this and will do it after this PR.

@grorp
Copy link
Member Author

grorp commented Apr 2, 2024

I tested my changes with ThreadSanitizer. It output countless of unrelated "data race" errors which are already present on master, but I found two warnings related to this PR that occur while shutting down:

ThreadSanitizer: data race (/mt/bin/minetest+0x55ca93) (BuildId: d5bfc2de556551e34bade641abe56a1f299abce2) in operator delete(void*)
ThreadSanitizer: data race /mt/src/settings.cpp:64:7 in SettingsHierarchy::getParent(int) const

tsan.txt

I think these are caused by the following situation:

  • FpsControl reads from g_settings on the main thread
  • Server::~Server deletes m_game_settings (calling g_settings->onLayerRemoved) on ServerStopThread

As far as I can see, fixing this problem would require implementing synchronization for g_settings, which I consider out of scope for this PR.

@sfan5 sfan5 self-assigned this Apr 3, 2024
@sfan5
Copy link
Member

sfan5 commented Apr 3, 2024

fyi I opened grorp#11

src/threading/lambda.h Outdated Show resolved Hide resolved
@grorp grorp changed the title Fix two common causes of "app isn't responding" [no squash] Fix two common causes of "app isn't responding" Apr 3, 2024
@sfan5
Copy link
Member

sfan5 commented Apr 3, 2024

Verify that loading of cached media isn't noticeably slower. (Obviously it'll be a bit slower since we're now drawing frames / handling events in-between media loading, but it shouldn't be noticeably slower.)
↑ Please actually test this. ↑

By the way, my results for this are 159ms (before) and 154ms (after). So not significant at all.

@grorp
Copy link
Member Author

grorp commented Apr 4, 2024

By the way, my results for this are 159ms (before) and 154ms (after). So not significant at all.

Thanks for testing, that's good. I had a big mistake in my measurement setup, even the old version wasn't as slow as I thought.

@sfan5 sfan5 merged commit fd8e021 into minetest:master Apr 5, 2024
13 checks passed
@grorp grorp deleted the free-the-main-thread branch April 8, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants