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

Remove busy loops #1367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove busy loops #1367

wants to merge 1 commit into from

Conversation

ilelann
Copy link

@ilelann ilelann commented Dec 5, 2021

replaced with either
this_thread::sleep_*
condition_variable for audio read/write loop

this reduces usage from "around 16%" (on a recent 16t CPU, thus a 2.5 threads load)
to a nicer "around 0.7%"

replaced with either
    this_thread::sleep_*
    condition_variable for audio read/write loop

this reduces usage from "around 16%" (on a 16t CPU, thus a 2.5 threads load)
to a nicer "around 0.7%"
@ilelann
Copy link
Author

ilelann commented Dec 5, 2021

Some missing include for Movie.cpp is killing linux/macos builds. I will fix that.

I probably also need to replicate AO's Movie.cpp fixes in AE.
I only tested AO but I will test AE soon.

Still interested in any feedback meanwhile.

@@ -1346,19 +1347,51 @@ EXPORT void CC SsSetTickMode_4FDC20(s32)
// Stub
}

// TODO: Removed 4FDC30
EXPORT bool CC SsSeqCalledTbyT_4FDC80_sleep_until_due_time_if_before_max_time(SYS_time_point_t current_time, SYS_time_point_t max_time)
Copy link
Contributor

Choose a reason for hiding this comment

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

_4FDC80 needs to be at the end of the function as exported function names are parsed at runtime for installing hooks into the game when running as a DLL. This name is super verbose too, I would have just left it as SsSeqCalledTbyT_4FDC80 tbh since the waiting/sleeping is an implementation detail.

Copy link
Author

Choose a reason for hiding this comment

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

I meant to avoid breaking existing SsSeqCalledTbyT_4FDC80 ABI with "max_time" new argument.
(because I understood that number-suffixed functions are meant to be interchangeable with original binaries)
Unless I miss something, I thus can simplify the name but not use the original one.

{
const auto nextDueTime = lastTime + 30;

if ((s32) (SYS_AsU32Ms(max_time) - nextDueTime) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

use static_cast instead of a C cast


const auto timeToWait = (s32) (nextDueTime - SYS_AsU32Ms(current_time));
Copy link
Contributor

Choose a reason for hiding this comment

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

use static_cast instead of a C cast

@@ -100,6 +100,11 @@ HRESULT SDLSoundSystem::Release()

// Stop audio rendering thread
mRenderAudioThreadQuit = true;
{
std::lock_guard g{mAudioRingBufferMutex};
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of 'g' can we at least called it 'lock' or something

@@ -139,6 +144,11 @@ void SDLSoundSystem::AudioCallBack(Uint8* stream, s32 len)
{
LOG_ERROR("Ring buffer read failure!");
}

{
std::lock_guard g{mAudioRingBufferMutex};
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto for naming


u32 SYS_AsU32Ms(SYS_time_point_t t)
{
return (u32) std::chrono::duration_cast<std::chrono::milliseconds>(t.time_since_epoch()).count();
Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast instead of C cast

@MrSapps
Copy link
Contributor

MrSapps commented Dec 5, 2021

We could probably do with someone with a slower machine testing this to check it doesn't impact performance. Maybe @mouzedrift can try one the build is fixed.

@mouzedrift
Copy link
Member

CPU usage went down to +-7% but the game looks very laggy

@MrSapps
Copy link
Contributor

MrSapps commented Dec 5, 2021

I suspected that might be an issue. @ilelann it might be better to try to get https://github.com/AliveTeam/sound_rev working instead as this can work without another thread which would remove the busy waits. It needs progressing so that the emulator integration can run the psx SPU code (instead of emulated mips code). Once this works it can be refactored/reduced down and integrated into relive.

@ilelann
Copy link
Author

ilelann commented Dec 5, 2021

Thanks a lot for your comments, I'll dig that.
@mouzedrift May I know what kind of hardware & OS was used for your test ?

@mouzedrift
Copy link
Member

windows 10 pro, 8 gb ram, intel core i5-4440 with intel hd graphics 4600

@MrSapps
Copy link
Contributor

MrSapps commented Jan 27, 2022

A temp hack fix was merged that removes the busy loops, already confirmed that this causes stuttering on some machines but it can be turned off via the ini. Hack fix is temp till audio is rewritten.

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.

3 participants