whisthq / whist

Whist Browser
https://whist.com
Apache License 2.0
7 stars 3 forks source link

Protocol gets crowded out on Windows #788

Closed philippemnoel closed 3 years ago

philippemnoel commented 3 years ago

Bug Description Make sure the Fractal protocol gets priority over other applications when it comes to rendering, by setting threads priority

Capture d’écran, le 2021-02-19 à 09 26 50
philippemnoel commented 3 years ago

So we set SDL high priority thread in the protocol, but I’m not sure exactly how effective that is, at least on Windows. I know, for having implemented it myself in the windows-service, that you can granularly control how much OS permission a Windows exe can take (you can even make it take more precedence than OS functions, which means if you do that Windows will start giving up before Fractal does, which is not recommended). It might be worth adding this priority-setting in Windows on top of the SDL thread priority to guarantee that Fractal takes priority. This is the answer (written in C# but the API is in C++ so we can easily call it from C) View in Slack

philippemnoel commented 3 years ago

this specifically sets the executable to run with #1 priority after OS-functions, which means it should be prioritized over other apps — I’m not sure how it plays out with other apps if they do the same, though View in Slack

philippemnoel commented 3 years ago

// above any other non-OS process on the computer (to avoid crowding out
// when the compuer reaches near 100% CPU utilization)
// See: <https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setpriorityclass>```
<sub>[View in Slack](https://tryfractal.slack.com/archives/G01313KKNF5/p1613742546003400)</sub>
philippemnoel commented 3 years ago

Actually — since we’ve forked SDL, the more clever option might to check how SDL_Thread_Priority_High is implemented in SDL and see if we can make it better ourselves View in Slack

philippemnoel commented 3 years ago

View in Slack

philippemnoel commented 3 years ago

so right now we set it to HIGH, but it could be set to TIME_CRITICAL <@U01125TJ6EB> <@USEKRDGH1> thoughts? View in Slack

philippemnoel commented 3 years ago

this was added in SDL 2.0.9, which is fairly recent View in Slack

philippemnoel commented 3 years ago

on MS-DOS systems it uses this function View in Slack

philippemnoel commented 3 years ago

Okay it seems to be handling pthreads very thoroughly but I am not convinced they are handling Windows threads as thoroughly View in Slack

philippemnoel commented 3 years ago

I think the two things we could do to improve here are: • Set our SDL threads to TIME_CRITICAL • Potentially modify SDL to better set thread priority on Windows with HIGH_PRIORITY_CLASS process View in Slack

philippemnoel commented 3 years ago
capture_d___e__cran__le_2021-02-19_a___08 58 46 capture_d___e__cran__le_2021-02-19_a___09 00 56
philippemnoel commented 3 years ago

context: it seems the main issue that @mingy98 was complaining about was in fact some latency introduced in the protocol via the loadingSDL function, which has been fixed. However, this does give us the opportunity to see that there is likely a better way to set thread priority by setting to TIME_CRITICAL, which we should do at least for our receiving thread.

philippemnoel commented 3 years ago

that issue was incorrectly scoped, it turned out to be a problem with our audio hotpath not being optimized. That being said, the REALTIME is still a valuable feature to add, and is scoped in #2144 so I'm closing this