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.

Sign up for GitHub

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

Jump to bottom

Progress Dialog: Fix race that could lead to ever-inaccurate results #14531

Merged
merged 2 commits into from Aug 22, 2023
Merged

Progress Dialog: Fix race that could lead to ever-inaccurate results #14531

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a3012c
Progress Dialog: Fix race that could lead to ever-inaccurate results
elad335 Aug 22, 2023
5380d32
Progress Dialog: Avoid PPU compilation pop-up on short linkage
elad335 Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 30 additions & 9 deletions rpcs3/Emu/system_progress.cpp
View file
Open in desktop
Expand Up @@ -117,15 +117,27 @@ void progress_dialog_server::operator()()
u32 pdone = 0;
auto text1 = text0;

const u64 start_time = get_system_time();

// Update progress
while (!g_system_progress_stopping && thread_ctrl::state() != thread_state::aborting)
{
const auto text_new = +g_progr.load();
auto whole_state = std::make_tuple(+g_progr.load(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ptotal, +g_progr_pdone);

while (true)
{
const auto new_state = std::make_tuple(+g_progr.load(), +g_progr_ftotal, +g_progr_fdone, +g_progr_ptotal, +g_progr_pdone);

if (new_state == whole_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

And why should that make any difference?
It may change a nanosecond later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before (race example in steps):

  1. Progress dialog loads nullptr text.
  2. Progress dialog loads ptotal. Let's say it is equal to X. pdone is equal to X as well here.
  3. A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
  4. Progress dialog loads pdone, pdone value in thread is now eqaul to X + 1, ptotal value in thread is X. In reality they are both X + 1.
  5. Progress dialog exists the loop, resets them all. now pdone is ever lagging by 1.

After:

  1. Progress dialog loads nullptr text.
  2. Progress dialog loads ptotal. Let's say it is equal to X. pdone is equal to X here as well here.
  3. A PPU thread is about to compile a module, sets text and incremenetes both ptotal and pdone.
  4. Progress dialog loads pdone, pdone value in thread is now eqaul to X + 1, ptotal value in thread is X. In reality they are both X + 1.
  5. Progress dialog sees value mismatch, restarts loads and gets equal ptotal and pdone and does not exit the loop.

Another example:
Before:

  1. Progress dialog loads nullptr text.
  2. A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
  3. Progress dialog loads pdone, pdone value in thread is now eqaul to X + 1, ptotal value in thread is X + 1. In reality they are both X + 1.
  4. Progress dialog loads ptotal. Let's say it is equal to X + 1. pdone is equal to X as well here.
  5. Progress dialog exists the loop, resets them all. now both pdone and ptotal are ever lagging by 1.

After:

  1. Progress dialog loads nullptr text.
  2. A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
  3. Progress dialog loads pdone, pdone value in thread is now eqaul to X + 1, ptotal value in thread is X + 1. In reality they are both X + 1.
  4. Progress dialog loads ptotal. Let's say it is equal to X + 1. pdone is equal to X as well here.
  5. Progress dialog sees value mismatch, restarts loads and gets equal ptotal and pdone and does not exit the loop.

TLDR: Thread-safety is a headache.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's purely random now, same as before, if this loop runs faster than the next change is supplied.
There's zero thread safety here.

You should've just used a struct with a mutex and call it a day.

Copy link
Contributor Author

@elad335 elad335 Aug 22, 2023 •
edited

Choose a reason for hiding this comment

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

No there is no race here now. The fact that SPU does not crash for games is a live example for it. GETLLAR is an atomic 128-byte load. This loop recheck is a technique to load atomicaly any amount of bytes without a mutex.

{
// Only leave while it has a complete (atomic) state
break;
}

whole_state = new_state;
}

const u32 ftotal_new = g_progr_ftotal;
const u32 fdone_new = g_progr_fdone;
const u32 ptotal_new = g_progr_ptotal;
const u32 pdone_new = g_progr_pdone;
const auto [text_new, ftotal_new, fdone_new, ptotal_new, pdone_new] = whole_state;

if (ftotal != ftotal_new || fdone != fdone_new || ptotal != ptotal_new || pdone != pdone_new || text_new != text1)
{
Expand All @@ -137,17 +149,20 @@ void progress_dialog_server::operator()()

if (!text_new)
{
// Close dialog
break;
// Incomplete state
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

}

if (show_overlay_message)
{
// Show a message instead
if (g_cfg.misc.show_ppu_compilation_hint)
// Show a message instead (if compilation period is estimated to be lengthy)
const u64 passed = (get_system_time() - start_time);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could've easily been optimized into another scope


if (pdone < ptotal && g_cfg.misc.show_ppu_compilation_hint && (pdone ? (passed * (ptotal - pdone) / pdone) : (passed * (ptotal + 1))) >= 100'000)
{
rsx::overlays::show_ppu_compile_notification();
}

thread_ctrl::wait_for(10000);
continue;
}
Expand Down Expand Up @@ -182,6 +197,12 @@ void progress_dialog_server::operator()()
});
}
}
// Leave only if total count is equal to done count
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

else if (ftotal == fdone && ptotal == pdone && !text_new)
{
// Complete state, empty message: close dialog
break;
}

if (show_overlay_message)
{
Expand Down