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

OpenTTD downloading from OneDrive freezes at startup #8978

Open
perezdidac opened this issue Apr 8, 2021 · 5 comments
Open

OpenTTD downloading from OneDrive freezes at startup #8978

perezdidac opened this issue Apr 8, 2021 · 5 comments
Labels
bug Something isn't working priority: low This issue should be fixed but has either a low impact or is en edge-case

Comments

@perezdidac
Copy link
Contributor

Version of OpenTTD

1.11

Windows 10

Expected result

OpenTTD starts

Actual result

OpenTTD starts, black screen, frozen cursor.

Thread blocked at

FILE *f = fopen(filename.c_str(), "rb");

in fileio.cpp.

filename is the name of the file being downloaded.

image

Observe OneDrive downloading stuff (I have no idea why) in Documents/OpenTTD:

image

That thread holds the game_state_mutex here:

image

So the main thread blocked at game_state_mutex:

image

and that's why we are frozen.

But the key here is that the fopen blocks until the file has been downloaded.

Steps to reproduce

I don't know why OneDrive is downloading content...

@nielsmh
Copy link
Contributor

nielsmh commented Apr 9, 2021

Your own screenshot has the answer to why OneDrive is involved: The path to your Documents folder is inside your OneDrive sync folder. As to why your files are not cached locally, I nobody here can answer that.
The same hang would happen if the files were on a hard disk drive that was spun down and needed to spin up before being able to read the file.

@PeterN
Copy link
Member

PeterN commented Apr 9, 2021

We could possibly test the file attributes to see if it's marked offline. However that might then end up with reports about files being ignored...

@nielsmh
Copy link
Contributor

nielsmh commented Apr 9, 2021

A better solution would be if all the file work was done in another thread than the UI, so the UI at least stays responsive if any file operation is slow.

@TrueBrain
Copy link
Member

A better solution would be if all the file work was done in another thread than the UI, so the UI at least stays responsive if any file operation is slow.

Some background for you all, so you get a bit more feeling where OpenTTD currently stands in terms of threading. As I think you mean the same as I am about to write, but the way you think about the solution might put you on the wrong track. Hopefully the following is going to help you out a bit:

At the moment, when using threads, there are always two threads worth mentioning:

  1. GameThread, handling the GameLoop once in a while (and the one hanging here)
  2. MainThread (or DrawThread), doing the UI drawing

Between those threads is a lock on the game-state. While the game-state is locked by either of the thread, the other cannot modify it in any way (for obvious reasons). How we currently make the NewGRF scanner responsive, is by releasing the lock on the game-state after each NewGRF that is loaded, giving the DrawThread a chance to make something appear on the screen (and some clever mechanism in the video-drivers makes sure they both get a turn).

In this instance, fopen is blocking, while having a lock on the game-state. In result, the DrawThread cannot produce a single frame, giving the appearance of the game being frozen.

A solution could be that we release the game-state a lot more often. For example, a lot in that function doesn't change the game-state at all. So if it would to release its lock on the game-state, the UI would happily continue to do its thing, giving the appearance everything is not hanging. The game itself of course still is, as we cannot continue till the scan is done anyway. But the mouse will be responsive, you can move windows, etc.

This is something new that we introduced in 1.11, and where we just started to get a feel of what we can and cannot do. It has a long way to go before we have a good feeling with it, but basically, locking the game-state less often will help enormously with how the game feels in terms of responsiveness. We also have the problem the other way around, that drawing the viewports keeps a lock on the game-state for far too long, meaning the next game-tick cannot happen because we are drawing. There is absolutely no need for that. But I digress.

The trick with releasing the game-state lock is that you don't want to do it too often, and lose performance on silly context-switches. So if someone would to look into this, a good look into what the GRF Scanner is doing and how / when it influence the game-state is a good first step. For example, in the viewport I noticed that if I add a small queue, I could easily do a lot of calculations outside the game-state lock. At the expensive of a bit of memory. But that is often a trade-of. I did not look into this specific function in detail, but I can imagine similar things going on here. Things like MD5 calculations, opening of file, reading of file, etc, all do not influence the game-state. If we can do all that first, while we do not hold a lock on the game-state, the NewGRF scanner will feel a lot more snappy.

Anyway, long story, sorry about that .. I hope it helps out in understanding a bit what a possible direction for a solution could be :)

@TrueBrain TrueBrain added bug Something isn't working priority: low This issue should be fixed but has either a low impact or is en edge-case labels Apr 10, 2021
@perezdidac
Copy link
Contributor Author

Wow thanks for the answers. I totally agree with @TrueBrain's one, this is a classic programming mistakes in all GUI apps/programs in general.

The main reason I wanted to raise this topic is because if I am experiencing this there is a high likelihood that others are, at least players with a plain Windows 10 installation as this OneDrive thing is on by default AFAIK.

As mentioned in above's comments, GameLoop() is executed after locking the game_state_mutex. GameLoop() then does lots of things obviously, one of them being ScanNewGRFFiles(), which is the one that ends up blocking here. I am not familiar with whether that can run safely without locking the game_state_mutex, but if it does that will make the fix straightforward, but I doubt it is safe as per now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: low This issue should be fixed but has either a low impact or is en edge-case
Projects
None yet
Development

No branches or pull requests

4 participants