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

Crippling performance regression on Windows #4801

Closed
TerminX opened this issue Oct 2, 2021 · 6 comments
Closed

Crippling performance regression on Windows #4801

TerminX opened this issue Oct 2, 2021 · 6 comments
Assignees
Milestone

Comments

@TerminX
Copy link

TerminX commented Oct 2, 2021

I'm seeing upwards of 10% CPU time spent in PeekMessage in user32.dll as a result of calling SDL_PollEvent() once per frame. Bisecting the problem reveals with 100% certainty that the bad commit is 0dd7024, "Modifies WaitEvent and WaitEventTimeout to actually wait instead of polling." While I can confirm a substantial amount of waiting is taking place, I'd wager none of it is intended.

If you need a test case, pull https://voidpoint.io/terminx/eduke32 and replace the copy of SDL provided in platform/Windows with the current revision (or 2.0.16, whatever). libSDL2.a and libSDL2main.a go in platform/Windows/lib/64 (assuming x86_64), and the headers go in platform/Windows/include/SDL2. This codebase builds with either MSYS2 or modern versions of Visual Studio. The resulting binary will run with the game data for either Duke Nukem 3D (any version) or Ion Fury. When I build the two static libraries, I pass --disable-libc to the configure script so I can use the same files with both MSYS2 and VS. I don't think this information is pertinent but felt it was worth mentioning as it should be the only "non-standard" thing we're doing.

The problem should immediately be obvious: disastrous latency spikes occur as soon as you start moving the mouse around. It's worse in-game, but you can see it in the menu, too. The worst I logged was over 500ms... in a release build, on a Ryzen 9 5950X. It's so bad that I'd think I must be using the library wrong if not for having used it successfully in the same codebase for over a decade.

I hope this is enough information to resolve the problem, or that if I'm doing something stupid on our end it's easy for me to fix. :)

@0x1F9F1
Copy link
Contributor

0x1F9F1 commented Oct 2, 2021

The issues with 0dd7024 should've been fixed in later commits (i.e b992b91), though that was before 2.0.16 was released.

Does it still happen on the latest main branch? If so, could you try replacing SDL_SendWakeupEvent with:

static int
SDL_SendWakeupEvent()
{
    return 0;
}

The actual problem is likely with how SDL_PollEvents interacts with high-frequency devices/events, but if 0dd7024 is the first commit where you are seeing the problem, the extra time spent locking/unlocking the mutex in SDL_SendWakeupEvent may be part of the issue.

Either way, also try #4794 since that should fix (avoid) the root of the issue.

@TerminX
Copy link
Author

TerminX commented Oct 3, 2021

image

Unfortunately, using the latest main with that function patched didn't really help. The input device in question does poll at 1000Hz.

@cgutman
Copy link
Collaborator

cgutman commented Oct 3, 2021

Can you post the performance analyzer output where you saw the PeekMessage() overhead? I would like to see what the exact call stack in SDL is for that.

@slouken slouken self-assigned this Oct 15, 2021
@slouken slouken added this to the 2.0.18 milestone Oct 15, 2021
@slouken
Copy link
Collaborator

slouken commented Oct 15, 2021

Please retest using the latest SDL snapshot at commit c583055:
http://www.libsdl.org/tmp/SDL-2.0.zip

I did a profiling session with a Razer Viper 8K mouse and saw roughly equivalent CPU usage before 0dd7024, after it, and after 8bf32e1. The vast majority of the CPU was in PeekMessageW in handleevents_pollsdl -> SDL_PollEvent -> SDL_PumpEvents -> WIN_PumpEvents -> PeekMessageW. It may be in my case the sheer number of mouse events overshadows the performance hit anywhere else.

Please let me know what you find!

@TerminX
Copy link
Author

TerminX commented Oct 15, 2021

Confirmed fixed.

@slouken
Copy link
Collaborator

slouken commented Oct 15, 2021

Thanks!

@slouken slouken closed this as completed Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants