-
-
Notifications
You must be signed in to change notification settings - Fork 946
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
Change: [Linkgraph] Pause the game when linkgraph jobs lag (#6470) #7081
Change: [Linkgraph] Pause the game when linkgraph jobs lag (#6470) #7081
Conversation
When does this pause happen? How can I reproduce it? |
Link-graph updates run on a snapshot taken at in-game time intervals, and have a deadline also given in in-game time. If the real-time processing takes longer than simulating the in-game time, the game stalls, or with this patch pauses. The easiest way to force a pause would probably be adding some sleeps into the link graph threads, I'm not sure what it would take to make a network sufficiently complex for link graph updates to be slow, but game simulation still run fast. Perhaps limiting the game to a single CPU core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this via inserting a CSleep(30000);
in LinkGraphSchedule::Run to artificially delay jobs.
Seems to work as intended, although I've only tested it in singleplayer.
It doesn't play nice with the "Link graph delay" line in the frame rate window, it always shows 0.03 ms or there about even when the job is delayed by 10+ seconds.
Is the wrong "Link graph delay" line a problem to be fixed or something obsoleted by this PR? |
I think you may as well remove the delay measurement entirely, since the main purpose of it was as an indicator for why the game might be freezing intermittently. Since this patch gives another indicator for this, the delay measurement is no longer necessary. |
Well, I just tested in in multi-player and I'm afraid to say it doesn't work right there. Doing the same simulation (CSleep) as @nielsmh I get a freeze and then directly consecutive pause and unpause messages. |
In multiplayer DoCommandP calls are executed on the next tick, and so the check needs to be done 2 ticks before the join tick instead of 1. Alternatively the feature could be made single-player only, which also seems reasonable. On the FPS window, hiding/removing the line when the feature is in operation seems reasonable. |
A few thoughts on this:
|
2c8597f
to
d2ed1ed
Compare
d2ed1ed
to
df958b2
Compare
This should now work correctly in multiplayer. The line in the FPS window should now only be present on network clients, where it is still useful.
Can you clarify what such a pause-like feature would be, and how it differs from pause?
This is not realistic given the current codebase and architecture. |
df958b2
to
a5697dd
Compare
I've just tested this again, and pausing works fine in network games. Unpausing OTOH is sometimes causing a (short) hang on the client side. I'm not sure if this is really fixable though, after all the client might simply be slower than the server (I've run server and client on the same PC for this test though). |
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
) Check if the job is still running two date fract ticks before it is due to join, and if so pause the game until its done. When loading a game, check if the game would block immediately due to a job which is scheduled to be joined within two date fract ticks, and if so pause the game until its done. This avoids the main thread being blocked on a thread join, which appears to the user as if the game is unresponsive, as the UI does not repaint and cannot be interacted with. Show if pause is due to link graph job in status bar, update network messages. This does not apply for network clients.
… on network clients Network servers and single player clients do not block on thread joins due to instead pausing shortly before the join is due.
a5697dd
to
e5c4486
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this PR is 2 years old, 2 developers were positive towards it and both reviewed it at least once, I am pretty certain this is doing what it should.
I rebased this PR, fixed some coding-style issues (comments too long, no dot at end of line, really minor stuff), and tested it. It does what it states it does.
This fix is, however, only a partial fix. If the linkgraph job is still running when exiting the game, it still hangs as if it is unresponsive. I am guessing that is really minor, but possibly future PRs could cancel the job in those scenarios or something.
Either way, tnx for the PR! Time to merge this after 2 years ... (sorry it took so long)
I've got this implemented in my branch JGRennison/OpenTTD-patches@375dbdb JGRennison/OpenTTD-patches@3f2c561 JGRennison/OpenTTD-patches@6bf4b67 In general I've got lots of linkgraph commits.
Not to worry :) |
Check if the job is still running one date fract tick before it is due
to join and if so pause the game until its done.
This avoids the main thread being blocked on a thread join, which appears
to the user as if the game is unresponsive, as the UI does not repaint
and cannot be interacted with.
Show if pause is due to link graph job in status bar, update network
messages.
This does not apply for network clients.