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

Do not put the main thread in realtime or high priority #7436

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

sakertooth
Copy link
Contributor

The main thread was set to run in real-time (or at high priority on Windows) to fix an annoying warning in 733491d. I'm not sure what warning would've popped up at the time that justifies doing something like this, but as of right now, this is definitely not something we should doing. The main thread has no business running in real-time and can potentially preempt the processing of the audio threads (which should actually be the threads at a higher priority) because Qt may want to do drawing.

Another major problem is that it seems like GNOME (maybe DE's or something else completely, not sure) enforce a limit on the amount of CPU time a real-time process can take. On my system, its 200ms. It shouldn't come as a surprise that the main thread needs a lot more time to do its work than that. It's allocating memory, loading up the Engine, loading plugins, etc. The problem with this is that if the main thread takes more than 200ms of CPU time at any given instance, it will be killed, causing the application to not even start.

@sakertooth
Copy link
Contributor Author

So it seems like GNOME recently started using rtkit with versions 45+, so this limitation seems to be from rtkit. I found this out from yabridge's README.

@Rossmaxx
Copy link
Contributor

Rossmaxx commented Aug 7, 2024

By main thread, i think you mean the thread used by Qt for the UI. If that's the case, shouldn't the audio thread be realtime? This might already be done, i just don't know if it's being done.

@sakertooth
Copy link
Contributor Author

sakertooth commented Aug 7, 2024

By main thread, i think you mean the thread used by Qt for the UI. If that's the case, shouldn't the audio thread be realtime? This might already be done, i just don't know if it's being done.

Yes. As mentioned, if anything, the audio threads should be in real-time, not the main thread.

Edit: We can work on making the audio threads real-time later, I just want to remove this for now.

Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@Rossmaxx Rossmaxx requested review from DomClark and PhysSong August 7, 2024 13:31
@enp2s0
Copy link
Contributor

enp2s0 commented Aug 7, 2024

can confirm that this resolves the issue with LMMS getting killed without a trace at startup. There is no reason to be running the main thread at realtime anyway, especially when the audio threads aren't (this is a separate issue which is not as simple to fix as just requesting RT in those threads, since we don't enforce RT requirements very well at all). This means we could even be preempting audio buffer processing so that QT can draw the UI, which is completely backwards. (I actually have noticed that when running near the limits of the CPU with large projects, scrolling the UI causes crackling and buffer overruns. Maybe this is the cause.)

As discussed in the Discord, the main thread gets killed whenever rtkit is running on the system, which manifests itself to the user as the splash screen appearing for a second and then disappearing without LMMS opening. In GNOME 45+ rtkit is used by the shell itself, causing the crashes on new versions of GNOME. rtkit is what imposes the 200ms limit on realtime threads.

@sakertooth sakertooth merged commit 632966c into LMMS:master Aug 7, 2024
11 checks passed
@sakertooth sakertooth deleted the fix-prio-main-thread branch August 7, 2024 17:12
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

Successfully merging this pull request may close these issues.

None yet

3 participants