Fix #129162: High idle CPU usage with PulseAudio in Linux #129312

A note for Sergey and Sebastian, the high CPU usage issue was introduced in Blender 4.3.
Joerg then commited a fix to main. I advised them that Blender 4.3 also needs the bug fix, and advised them to open a PR (as it's best to go through review for changes to 4.3 this late in the development cycle)

As a result of this there will be merge conflicts when merging 4.3 into main after this change is commited to 4.3.

I have added Sergey and Sebastian as reviewers because they're more familiar with this area. A note for Sergey and Sebastian, the high CPU usage issue was introduced in Blender 4.3. Joerg then commited a fix to main. I advised them that Blender 4.3 also needs the bug fix, and advised them to open a PR (as it's best to go through review for changes to 4.3 this late in the development cycle) As a result of this there will be merge conflicts when merging 4.3 into main after this change is commited to 4.3.

Sergey Sharybin reviewed 2024-10-22 14:54:10 +02:00

Sergey Sharybin left a comment

I wouldn't be able to test this, as I don't have an easy access to Linux machines at this time.

The PR would definitely benefit from more explanation of what's going on, and what the fix actually does.
One of the strange things to me is a non-guarded access and write to the m_corked variable (which seems it can get out of sync with the parameter passed to AUD_pa_stream_cork.

I wouldn't be able to test this, as I don't have an easy access to Linux machines at this time. The PR would definitely benefit from more explanation of what's going on, and what the fix actually does. One of the strange things to me is a non-guarded access and write to the `m_corked` variable (which seems it can get out of sync with the parameter passed to `AUD_pa_stream_cork`.

Joerg Mueller commented

2024-10-22 15:51:46 +02:00

I wouldn't be able to test this, as I don't have an easy access to Linux machines at this time.

The PR would definitely benefit from more explanation of what's going on, and what the fix actually does.

Added a short description.

One of the strange things to me is a non-guarded access and write to the m_corked variable (which seems it can get out of sync with the parameter passed to AUD_pa_stream_cork.

How is it unguarded? Please check again. All accesses to m_corked are properly guarded with a lock_guard!

> I wouldn't be able to test this, as I don't have an easy access to Linux machines at this time. > > The PR would definitely benefit from more explanation of what's going on, and what the fix actually does. Added a short description. > One of the strange things to me is a non-guarded access and write to the `m_corked` variable (which seems it can get out of sync with the parameter passed to `AUD_pa_stream_cork`. How is it unguarded? Please check again. All accesses to `m_corked` are properly guarded with a `lock_guard`!

Sergey Sharybin commented

2024-10-24 07:57:18 +02:00

Added a short description.

Thanks!

All accesses to m_corked are properly guarded with a lock_guard!

Ok, I've missed that! What seemed weird to me is that the AUD_pa_stream_cork() is guarded by the lock on m_mainloop, while m_corked = true; is not:

std::lock_guard<ILockable> device_lock(*this);
...
AUD_pa_threaded_mainloop_lock(m_mainloop);
AUD_pa_stream_cork(m_stream, 1, nullptr, nullptr);
AUD_pa_stream_flush(m_stream, nullptr, nullptr);
AUD_pa_threaded_mainloop_unlock(m_mainloop);
m_corked = true;

Keep in mind, not saying it is mistake in the code, just something that seemed a bit odd. If you confirm it's actually all fine -- I'm fine with it too!

> Added a short description. Thanks! > All accesses to `m_corked` are properly guarded with a `lock_guard`! Ok, I've missed that! What seemed weird to me is that the `AUD_pa_stream_cork()` is guarded by the lock on `m_mainloop`, while `m_corked = true;` is not: ``` std::lock_guard<ILockable> device_lock(*this); ... AUD_pa_threaded_mainloop_lock(m_mainloop); AUD_pa_stream_cork(m_stream, 1, nullptr, nullptr); AUD_pa_stream_flush(m_stream, nullptr, nullptr); AUD_pa_threaded_mainloop_unlock(m_mainloop); m_corked = true; ``` Keep in mind, not saying it is mistake in the code, just something that seemed a bit odd. If you confirm it's actually all fine -- I'm fine with it too!

Joerg Mueller commented

2024-10-24 09:41:25 +02:00

Yep, its' alright. The pa main loop only needs to be locked during the flush and cork calls. It doesn't matter if m_corked is written while it's locked or not as it is not accessed by the main loop.

I'm always trying to lock as briefly as possible though a bool assignment likely has little to no impact. That's why I put it outside.

Yep, its' alright. The pa main loop only needs to be locked during the flush and cork calls. It doesn't matter if `m_corked` is written while it's locked or not as it is not accessed by the main loop. I'm always trying to lock as briefly as possible though a bool assignment likely has little to no impact. That's why I put it outside.

Sergey Sharybin approved these changes 2024-10-25 07:36:12 +02:00

Sergey Sharybin left a comment

Thanks for checking and explaining!

TL;DR: I think it's better to land the fix soon.

We are currently at the Blender conference, and I don't have access to a Linux box to test. But I'd trust your tests! Since it is for 4.3, it's better to land it soon, so that we have the best chance of people trying it out before the release (for the unlikely scenario we've missed something).

Thanks for checking and explaining! TL;DR: I think it's better to land the fix soon. We are currently at the Blender conference, and I don't have access to a Linux box to test. But I'd trust your tests! Since it is for 4.3, it's better to land it soon, so that we have the best chance of people trying it out before the release (for the unlikely scenario we've missed something).

Sebastian Parborg approved these changes 2024-10-29 16:17:57 +01:00

Sebastian Parborg left a comment

LGTM

Sebastian Parborg merged commit 97e9a722fc into blender-v4.3-release

2024-10-29 16:19:22 +01:00

Sebastian Parborg referenced this issue from a commit

2024-10-29 16:19:22 +01:00

Fix #129162: High idle CPU usage with PulseAudio in Linux

Sign in to join this conversation.

No reviewers