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
Progress Dialog: Fix race that could lead to ever-inaccurate results #14531
Conversation
elad335
commented
Aug 22, 2023
•
edited
edited
- Fix state atomicity: this is similar to how SPU GETLLAR works by loading all the values again and comparing to old state to check for whole load-atomicity of multiple values in a lock-free manner.
- Avoid PPU compilation pop-up on short linkage.
bbe00f0
to
d4db464
Compare
f38dac0
to
5711a00
Compare
// Close dialog | ||
break; | ||
// Incomplete state | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
@@ -182,6 +197,12 @@ void progress_dialog_server::operator()() | |||
}); | |||
} | |||
} | |||
// Leave only if total count is equal to done count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
{ | ||
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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):
- Progress dialog loads nullptr text.
- Progress dialog loads ptotal. Let's say it is equal to X. pdone is equal to X as well here.
- A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
- 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.
- Progress dialog exists the loop, resets them all. now pdone is ever lagging by 1.
After:
- Progress dialog loads nullptr text.
- Progress dialog loads ptotal. Let's say it is equal to X. pdone is equal to X here as well here.
- A PPU thread is about to compile a module, sets text and incremenetes both ptotal and pdone.
- 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.
- Progress dialog sees value mismatch, restarts loads and gets equal ptotal and pdone and does not exit the loop.
Another example:
Before:
- Progress dialog loads nullptr text.
- A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
- 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.
- Progress dialog loads ptotal. Let's say it is equal to X + 1. pdone is equal to X as well here.
- Progress dialog exists the loop, resets them all. now both pdone and ptotal are ever lagging by 1.
After:
- Progress dialog loads nullptr text.
- A PPU thread is about to compile a module, sets text and incrementes both ptotal and pdone.
- 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.
- Progress dialog loads ptotal. Let's say it is equal to X + 1. pdone is equal to X as well here.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
// 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); |
There was a problem hiding this comment.
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