Use NET_Sleep() (or Sys_Sleep in SP) to avoid busy-waiting

[smcv]

Backported and simplified from an assortment of ioquake3 commits,
mostly by Thilo Schulz, with contributions from Zack Middleton and
Özkan Sezer.

Fixes #507.

---

I've finally assembled enough of a build system (mingw-w64, #688) and test system (a spare laptop) to test this on Windows as well as Linux. It seems to work fine either way:

• test systems: Debian Linux on Lenovo X220, Windows 7 on Lenovo X201 (both 2 cores × 2 threads, Intel integrated graphics)
• steps to test:
  • watch CPU load with Task Manager on Windows or top on Linux
  • start JA SP (I used the autosave at the beginning of yavin1b) or MP (I used "Create Game" with no bots on Tatooine)
  • navigate to a location where something is animated (I used the river in yavin1b and a spinning Holocron in MP)
  • cg_drawfps 1
  • com_busywait
  • com_maxfps
  • set com_maxfps 5
  • set com_maxfps 125
  • set com_busywait 1
  • set com_maxfps 5
  • set com_maxfps 125
• expected results:
  • default com_maxfps is 125, default com_busywait is 0
  • with com_maxfps 5, framerate and CPU usage are noticeably lower
  • with com_maxfps 125, framerate and CPU usage both go back up
  • with com_busywait 1, CPU usage is close to 100% of 1 thread
• actual results: as expected. Representative numbers from Windows on my X201:
  • com_busywait 0, com_maxfps 5 => 4-5 fps, 8-12% of 1 core
  • com_busywait 0, com_maxfps 30 => 30 fps, 40% of 1 core
  • com_busywait 0, com_maxfps 125 => 80-85 fps, 100% of 1 core (default settings)
  • com_busywait 1, com_maxfps 5 => 4-5 fps, 100% of 1 core
  • com_busywait 1, com_maxfps 30 => 30 fps, 100% of 1 core
  • com_busywait 1, com_maxfps 125 => 80-85 fps , 100% of 1 core

(The X220 has a better CPU and graphics, and only needs ~70% of a core with the default settings.)

I don't have OS X, but OS X uses basically the same code paths as Linux, so I expect it to work there.

---

Summarizing negative comments on this change from #507:

• @ensiform said that this change broke com_maxfps in MP at some point. I think my test results indicate that the SDL2 port fixes this? Or if there is some other situation that breaks, please describe it - one possible mitigation would be to default com_busywait to 1 on Windows and 0 elsewhere.
• @ensiform pointed out that this patch results in com_minimized and com_unfocused existing in the dedicated server (with their values always 0). This matches what ioquake3 does, and I think it makes the code more maintainable; but if people have a strong enough objection to this to consider it a merge blocker, I can sprinkle #ifdefs around it.

[xycaleth]

This works as expected on my Windows computer! However, testing on OS X, I'm not finding any difference between using com_busyWait and not using it.

@ensiform pointed out that this patch results in com_minimized and com_unfocused existing in the dedicated server (with their values always 0). This matches what ioquake3 does, and I think it makes the code more maintainable; but if people have a strong enough objection to this to consider it a merge blocker, I can sprinkle #ifdefs around it.

I prefer having the extra cvars around and not setting them for the dedicated server. But I don't see these changes in your PR. Was this intentional?

[smcv]

However, testing on OS X, I'm not finding any difference between using com_busyWait and not using it.

Odd... I would have expected OS X to behave essentially the same as Linux here, it's using the same source code after all. How new is your Mac? If you turn com_maxfps down to some stupidly low value (I used 5fps) do you start to see a difference?

OS X is the platform I can't currently test on - I have an old Mac which can just about manage Q3-engine games, but it's currently Linux-only. I'll see if I can find OS X installation media. Hopefully a self-compiled SDL and OpenJK will work on something as ancient as 10.2.

I prefer having the extra cvars around and not setting them for the dedicated server. But I don't see these changes in your PR.

• smcv@b2a4da0#diff-4ede2698b1a157831c62c8b2be626bb2L61
• smcv@b2a4da0#diff-a0d92c7bd8200af92ee6c70fde55120fL17
• smcv@b2a4da0#diff-a0d92c7bd8200af92ee6c70fde55120fL135

[dionrhys]

It seems to work well for me on Windows. It would be nice if com_maxfpsUnfocused and com_maxfpsMinimized matched the retail JKA equivalent of 5ms sleeps at the main loop level by default. Though fixed sleeps per frame isn't the same as limiting the max FPS, so it would need different logic. (CPU usage would drop dead when the game isn't in focus then)

The changed code regarding network packet handling makes me wary of regression, but that's me being paranoid I guess. The new net_dropsim cvar is initialized differently to how it was before though; default value "" and CVAR_TEMP instead of retail JKA default value "0" and CVAR_CHEAT (which made a bit more sense).

[ensiform]

Would really like to see this get tested and working if someone can check all OS'

@dionrhys it doesn't have to retain the same logic because I don't see why Sleep should be necessary if its throttling while minimized anyway?

@smcv are the stdin checks in NET_Sleep still necessary in this? They don't exist in ioq3 and is handled in CON_Input functions.

[ensiform]

There seems to also be some discrepencies in the stdin code even without this pull request. stdin_active is defined in sys_unix.cpp which is what is extern'd in net_ip but its never assigned to anything beyond the initial qtrue. There is a file local stdin_active in con_tty.cpp which is used in that file however no default value is set (presumably defaults to qfalse 0 because its a global)

[smcv]

Would really like to see this get tested and working if someone can check all OS'

The patch has conflicts with current master, but they look easy to resolve; I'll try to re-test sometime soon. I can test Linux and (with some difficulty) Windows, but not OS X. If you're particularly concerned we could make com_busywait default to 1 (equivalent to the old behaviour) on OS X.

It would be nice if com_maxfpsUnfocused and com_maxfpsMinimized matched the retail JKA equivalent of 5ms sleeps at the main loop level by default.

I think we should either leave them at 0 by default (so unfocused and minimized have the same 125fps framerate cap as normal gameplay), or set something actually noticeable.

If retail JKA's logic was essentially

    while (1) {
        render 1 frame, however long that takes
        sleep 5ms
    }

then that doesn't actually alter the framerate much. Suppose your computer could produce f fps without the sleep. With the sleep, it would produce f' = 1/(0.005 + 1/f) fps instead. This doesn't actually make a whole lot of difference, unless your framerate is already so high that it's rendering wasted frames when outputting to a typical 60Hz display:

f    | f'
20  | 18.2
30  | 26.1
50  | 40.0
60  | 46.2
100 | 66.7
200 | 100

The changed code regarding network packet handling makes me wary of regression

It's as similar as I could get it to ioquake3 - that seemed more likely to be good than trying to stay as close as possible to late 90s Quake 3 engine technology :-)

The new net_dropsim cvar is initialized differently to how it was before though; default value "" and CVAR_TEMP instead of retail JKA default value "0" and CVAR_CHEAT (which made a bit more sense).

You're right that 0 makes more logical sense, but this is how I found it (in ioquake3). "" is effectively interpreted as 0 anyway, when being parsed as a float (it's put through atof() which has no error-detection, and just returns 0 for non-numbers).

I'm not sure valuable it is to treat simulated packet loss as cheating; if it was a viable way to cheat, people could equally easily do enough irrelevant traffic in parallel to cause non-simulated packet loss :-)

There seems to also be some discrepencies in the stdin code even without this pull request

Indeed, well spotted. I think the intention may have been that all the uses of stdin_active were the same variable, which would be set true if standard input was not a terminal (a pipe or socket or something). If that had been done correctly, then the practical effect of monitoring stdin in NET_Sleep would be that any pending input on a non-interactive standard input would wake the engine up to process it during the idle time between frames, instead of remaining asleep until either there's new network data or it's time to start work on the next frame.

I don't think that actually matters in practice: the input isn't lost, gets queued up in the kernel or in the sending process instead of in the Q3 engine, and is processed "soon". If we're running at, say, 10fps or better, then stdin input wouldn't have to wait more than 100ms to be processed, which seems fine. Entering text into the server console is rarely time-critical :-)

In ioquake3, it seems to have worked the way I think it was intended to in the original id Software code-drop, then became the same #ifdef as in current OpenJK at some later point, then started to always monitor stdin in 672cfbf1 "Merge unified-sdl to trunk" (unfortunately the full history has been lost by the svn-to-git conversion). The same merge seems to have broken the intended functionality of stdin_active by making the version of it in the console code static.

Since e46fe244 "rewrite of the win32 dedicated console" in 2007, the #ifdef was replaced by just never monitoring stdin. I think we can probably conclude from the lack of a fix in the following 8 years that it was never actually very important, so you're probably right that I should drop that part.

[ensiform]

Also in ioquake3, Sys_Sleep is never called except in cases of no mapname during SV_Frame.

[mrwonko]

I can test on mac and Windows.

[Razish]

I can test on Mac and Linux :p

[smcv]

Also in ioquake3, Sys_Sleep is never called except in cases of no mapname during SV_Frame.

Right, but ioquake3 is always network-capable, whereas JASP/JK2SP is single-player-only.

If you'd prefer me to link the MP netcode into SP, and replace the call to Sys_Sleep() in SP by a call to NET_Sleep() with no socket to monitor, that would work - it would effectively be emulating sleep() with a call to select() with empty fd_sets, which is a standard Unix idiom.

[ensiform]

My mistake, Sys_Sleep is still used in main() in our codebase but not in ioquake3 if dedicated or minimized.

[smcv]

Sys_Sleep is still used in main() in our codebase

Ah, I think I might understand what you meant by this now: the Windows main() still has the hard-coded Sleep( 5 ) and/or Sleep( 50 ), even though my changes in this PR should make it unnecessary. I'll remove that.

[smcv]

Branch updated and seems to work as intended on Windows (10, cross-compiling with mingw on Linux) and Linux. @mrwonko, @Razish: please test on Mac?

(My branch at https://github.com/smcv/OpenJK/tree/sleep%2Btravis is what I actually tested, because I can't compile on mingw without also fixing #741, but the one proposed here just has the sleep-related commits.)

It would be nice if com_maxfpsUnfocused and com_maxfpsMinimized matched the retail JKA equivalent of 5ms sleeps at the main loop level by default. Though fixed sleeps per frame isn't the same as limiting the max FPS, so it would need different logic. (CPU usage would drop dead when the game isn't in focus then)

As described in my previous comment, the effect of a 5ms sleep per frame varies according to what your uncapped framerate would have been, so there is no value for com_maxfpsWhatever that can have exactly this effect for everyone? I've arbitrarily chosen to cap at 50fps, which is approximately what the result would be if your uncapped framerate was a little less than 60fps. If you don't like 50 as a default, it's probably best to choose a value that is a divisor of 1000: 10, 20 or 25 might make sense.

stdin_active is defined in sys_unix.cpp which is what is extern'd in net_ip but its never assigned to anything beyond the initial qtrue.

Well spotted. I've removed this one from both sys_unix and net_ip.

(I think it would be a good goal to remove all extern declarations from *.c, *.cpp: if something in a different module is API then it should be in a header file, and if it isn't API it shouldn't be used via explicit extern.)

There is a file local stdin_active in con_tty.cpp which is used in that file however no default value is set (presumably defaults to qfalse 0 because its a global)

I've left this one as-is, it seemed more or less correct. Static variables (file-local) are implicitly initialized to zero by the compiler/linker.

the Windows main() still has the hard-coded Sleep( 5 ) and/or Sleep( 50 ), even though my changes in this PR should make it unnecessary. I'll remove that.

I've left it in the shared main(), but made it conditional on com_busyWait 1 (so you get the 5ms pauses if you are in the old-style code path, but not if you are in the framerate-capped code path). Does that make sense?

The Windows-specific copies of main() were in files that turn out not to have even been compiled, so I deleted those files to reduce confusion.

[Razish]

Perfect timing, I will test this and review the Clang breakage too tomorrow.
Thanks.

[Razish]

I have confirmed com_busyWait and com_maxFPS*** working perfectly (WRT CPU usage and reported framerate) on OS X Yosemite built with Clang (also incorporating pr/741 to fix the build)

So that's Windows, Linux and Mac confirmed working? If so, I am very happy to merge this :)

[ensiform]

With busy wait 0?

[Razish]

With all permutations of the above cvars, working as intended.

[ensiform]

The fps cap works as intended?

[Razish]

Yep.

[ensiform]

And on Windows?

[Razish]

Haven't tested, I don't really have time/effort at home to boot into Windows and test there lately.

[ensiform]

Windows is where the issue was afaik. When I was initially working on this that was my issue on Windows.

[smcv]

that was my issue on Windows

Sorry, what is "that" here? I've lost track of which configuration you've had trouble with.

[ensiform]

Having the new behavior has issues long ago.

[smcv]

Having the new behavior has issues long ago.

Sorry, I can't test for "issues". Please could you be more specific?

If the test steps from my first comment aren't sufficient to cover the situations that you're concerned about, please describe the configurations/situations that have had problems in the past, and what those problems were.

[ensiform]

The FPS cap was no longer being affected (com_maxFPS) with the new behavior (busyWait 0)

[smcv]

The FPS cap was no longer being affected (com_maxFPS) with the new behavior (busyWait 0)

Just to be sure, I've re-tested all combinations of:

• com_maxfps 10, 60 or 125
• com_busywait 0 or 1
• Windows (32-bit OpenJK on 64-bit Windows 10, Lenovo X201) or Linux (64-bit Debian 8, Lenovo X220)

with the multiplayer engine, and it still seems to work: the framerate is approximately at the cap, when I reduce the cap to 10fps the animation is visibly bad (as I'd expect), and with com_busywait 0 the lower cap results in lower CPU use.

(With a 60fps cap I actually get 62-63fps, which is because the engine converts frames per second into milliseconds per frame, and 1000 isn't evenly divisible by 60.)

I've also re-tested the single-player engine with com_busywait 0 on Windows, and that behaves as expected at the three framerates I tried.

[ensiform]

Then there's no issues anymore.

[Razish]

Hooray!

[ensiform]

@dionrhys proposes we back out of this until the CPU usage regression in servers is fixed.

[dionrhys]

Sorry, I didn't realise the original method had explicit Sleeps of 5 milliseconds. This would explain the "regression" 😉

[smcv]

@dionrhys, @ensiform: are you sufficiently happy with my explanation on #823 to not want to back this out any more?

The thing I primarily wanted to fix is that before merging #695, on a client system capable of rendering at more than cl_maxfps, OpenJK would push the CPU usage right up to 100% of a core by busy-waiting. If you do that for too long on a laptop, you'll tend to run into CPU throttling where the firmware forces the clock speed right down (to avoid hardware damage or user injury), resulting in performance abruptly dropping right down - the opposite of what we wanted. A framerate cap is a sensible way to avoid that.

My preference order for our options for how to deal with this goes something like:

• keep the current implementation, performance/load resembles ioquake3 (preferred)
• merge something similar to smcv/OpenJK@e1ee901 (optionally reinstate the sleep hack) and document the new cvar
• think of something else that meets whatever additional requirements you have
• back out Use NET_Sleep() (or Sys_Sleep in SP) to avoid busy-waiting #695

I would appreciate it if people who are concerned about this could try smcv/OpenJK@e1ee901 with com_dedicated_sleep set to 5, as described on #823.

[dionrhys]

I think this is suitable as-is right now. If someone comes along and finds any optimisation whatsoever in this implementation in either ioq3 or OpenJK, we can be sure to integrate that in :)

[Razish]

I have a bright idea - how about we profile the whole code and optimise it to reduce CPU usage :P