| Summary: | [PATCH] SDL_WaitEventTimeout() delays much longer than timeout | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Cameron Gutman <cameron.gutman> |
| Component: | events | Assignee: | Ryan C. Gordon <icculus> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | icculus, sezeroz |
| Version: | 2.0.9 | Keywords: | target-2.0.12 |
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | Reduce delay in SDL_WaitEventTimeout | ||
I forgot to mention: if you go with the alternative fix, please update the documentation for SDL_WaitEvent() to clearly indicate this potential performance issue. It took some non-trivial debugging/profiling for me to figure out where the mysterious 100 FPS rendering and 100 Hz input polling cap was coming from. (Sorry if you get several emails like this, we're marking a bunch of bugs.) We're hoping to ship SDL 2.0.11 on a much shorter timeframe than we have historically done releases, so I'm starting to tag bugs we hope to have closed in this release cycle. Note that this tag means we just intend to scrutinize this bug for the 2.0.11 release: we may fix it, reject it, or even push it back to a later release for now, but this helps give us both a goal and a wishlist for the next release. If this bug has been quiet for a few months and you have new information (such as, "this is definitely still broken" or "this got fixed at some point"), please feel free to retest and/or add more notes to the bug. --ryan. We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc). As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change! Thanks, --ryan. We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc). As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change! Thanks, --ryan. I'm putting in this change from a 10ms delay to 1ms, because it couldn't hurt (unless a platform treats a 1ms delay as a busy-loop, but we'll let them file separate bugs), but this function some day needs to stop looping and just do some gigantic equivalent of WaitForMultipleObjects()/select(), and let the OS put it to sleep until exactly when there's something new to report. The delay was reduced in https://hg.libsdl.org/SDL/rev/a429ccdea379 --ryan. (Also, my apologies, I just changed the value without using your actual patch, which had a much more detailed commit message, not to mention personal credit to you. Sorry about that!) --ryan. Better comment and attribution committed! https://hg.libsdl.org/SDL/rev/830cadcdc1a2 |
Created attachment 3443 [details] Reduce delay in SDL_WaitEventTimeout SDL_WaitEventTimeout() currently has a hardcoded SDL_Delay(10) that runs if the timeout hasn't yet expired. This is problematic because the caller of SDL_WaitEventTimeout() may want a wait significantly shorter than that. If the caller asks for a 1 ms timeout and no events are ready, the function will return in 10 ms. In addition to the direct effect on SDL_WaitEventTimeout(), SDL_WaitEvent() uses SDL_WaitEventTimeout() with a timeout of -1. This means any application that doesn't want to poll (or has nothing useful to do without events available) can't use SDL_WaitEvent() unless they are willing to deal with the hidden 10 ms sleep inside it. This sleep causes dropped frames for apps with 6+ ms rendering times and caps the effective event loop polling rate using SDL_WaitEvent() or SDL_WaitEventTimeout() at about 100 Hz. To avoid these issues, these applications have to roll their own SDL_WaitEvent() loop with SDL_PollEvent() and SDL_Delay(1). The attached patch fixes both issues by changing the SDL_Delay from 10 ms to 1 ms. The CPU usage effects aren't noticeable in my testing. Alternative fix: If you feel strongly that SDL_WaitEvent() should wait 10 ms between iterations, we can change SDL_WaitEventTimeout() to pass the remaining timeout to SDL_Delay() instead of always passing 10.