mirror of
https://github.com/hrydgard/ppsspp.git
synced 2026-10-01 14:58:14 +00:00
Try to not spin so hard in the debugger when stepping
Core_ProcessStepping() returns immediately when the CPU is stopped with nothing queued, so Core_RunLoopUntil() returns immediately, so whatever drives it comes straight back. headless does that in a loop with no frame pacing at all, so a paused emulator sat at 100% of a core: measured 6.02 CPU-seconds over 6 wall seconds parked at startBreak. A debugger session is stopped most of the time, so this also dominated any profile taken of one - showing up as synchronization overhead around Core_RunOnCPUThread, which was just the hottest thing inside the spin rather than a problem with the queue. The CPU thread now blocks on a condition variable in that case. Anything that gives it something to do wakes it - Core_RunOnCPUThread() on push (with the queue mutex held, so it can't sleep on a task already queued), Core_RequestCPUStep(), and Core_Resume() - so the 2ms timeout is only a backstop for state changed without a wake, never how work is normally noticed. The wait is deliberately short rather than indefinite: callers do real work after Core_RunLoopUntil() returns, and in the app build that includes rendering the ImGui debugger from this same thread, so this has to bound how long a paused frame takes rather than replace the frame loop. Now 0.05 CPU-seconds over the same 6 seconds. No measurable cost to anything else: an identical scripted boot runs in 2514ms vs 2476ms before, and 20 consecutive cpu.stepInto still complete promptly. 55 unit tests pass, 314/314 pspautotests with --graphics=software. Also: wsdbg's README claimed a raw JSON line gets a ticket auto-assigned when it lacks one. It doesn't - the code deliberately sends raw lines exactly as written, and omitting the ticket is how you say "not waiting for an answer". Corrected. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
This commit is contained in:
1 parent
e46a853bc7
commit
6175fab373
3 files changed
+66
-6
No files matched your search
@@ -109,6 +109,46 @@ static std::thread::id g_cpuThreadId;
|
||||
// without taking g_cpuQueueMutex - g_cpuThreadId itself never changes once this becomes true.
|
||||
static std::atomic<bool> g_cpuThreadIdValid{ false };
|
||||
|
||||
// Bumped by anything that gives a paused CPU thread something to do - a queued task, a step
|
||||
// request, a resume. Core_IdleWaitWhileStepping() blocks on this rather than spinning; see there.
|
||||
static std::mutex g_idleMutex;
|
||||
static std::condition_variable g_idleCond;
|
||||
static u64 g_idleWakeCounter = 0;
|
||||
|
||||
void Core_WakeIdleCPUThread() {
|
||||
{
|
||||
std::lock_guard<std::mutex> guard(g_idleMutex);
|
||||
g_idleWakeCounter++;
|
||||
}
|
||||
g_idleCond.notify_all();
|
||||
}
|
||||
|
||||
// Called on the CPU thread when it's stopped and has nothing queued. Without this the whole
|
||||
// pause is a busy-wait: Core_ProcessStepping() returns immediately when idle, so Core_RunLoopUntil()
|
||||
// returns immediately, so whatever drives it goes straight back round. headless does that with no
|
||||
// frame pacing whatsoever, which measured at a full core burned for as long as the CPU stayed
|
||||
// stopped - and since a debugger session is stopped most of the time, it dominates any profile
|
||||
// taken of one (as sync overhead around Core_RunOnCPUThread, which is simply the hottest thing in
|
||||
// the spin).
|
||||
//
|
||||
// The wait is deliberately short rather than indefinite. Callers do real work after we return -
|
||||
// most importantly the app build renders the ImGui debugger from this same thread - so this must
|
||||
// bound how long a paused frame takes, not replace the frame loop. Anything that actually wants
|
||||
// the CPU thread also calls Core_WakeIdleCPUThread(), so the timeout is only a backstop for state
|
||||
// changed without one, never the normal path to noticing work.
|
||||
static void Core_IdleWaitWhileStepping() {
|
||||
constexpr auto kMaxIdleWait = std::chrono::milliseconds(2);
|
||||
{
|
||||
// Don't sleep if something is already waiting on us.
|
||||
std::lock_guard<std::mutex> guard(g_cpuQueueMutex);
|
||||
if (!g_cpuQueue.empty())
|
||||
return;
|
||||
}
|
||||
std::unique_lock<std::mutex> guard(g_idleMutex);
|
||||
const u64 seen = g_idleWakeCounter;
|
||||
g_idleCond.wait_for(guard, kMaxIdleWait, [&] { return g_idleWakeCounter != seen; });
|
||||
}
|
||||
|
||||
void Core_RunOnCPUThread(std::function<void()> func) {
|
||||
if (g_cpuThreadIdValid.load(std::memory_order_acquire) && std::this_thread::get_id() == g_cpuThreadId) {
|
||||
// Already on the CPU thread (or called before it's ever run) - just do it now, avoids deadlock.
|
||||
@@ -121,6 +161,12 @@ void Core_RunOnCPUThread(std::function<void()> func) {
|
||||
|
||||
std::unique_lock<std::mutex> guard(g_cpuQueueMutex);
|
||||
g_cpuQueue.push_back(task);
|
||||
if (!System_GetPropertyBool(SystemProperty::SYSPROP_IS_HEADLESS)) {
|
||||
// Do this with g_cpuQueueMutex held: the CPU thread checks that queue before it decides to
|
||||
// sleep, so waking it after the push (and before we block) can't leave it asleep on a task
|
||||
// that's already there.
|
||||
Core_WakeIdleCPUThread();
|
||||
}
|
||||
g_cpuQueueCond.wait(guard, [&] { return task->done; });
|
||||
}
|
||||
|
||||
@@ -369,6 +415,11 @@ void Core_RunLoopUntil(u64 globalticks) {
|
||||
if (coreState == CORE_REENTER_DISPATCH) {
|
||||
coreState = preState;
|
||||
}
|
||||
// Still stopped with nothing pending, so block briefly instead of handing straight
|
||||
// back to a caller that will just call us again - see Core_IdleWaitWhileStepping().
|
||||
if (coreState == CORE_STEPPING_CPU || coreState == CORE_STEPPING_GE) {
|
||||
Core_IdleWaitWhileStepping();
|
||||
}
|
||||
return;
|
||||
}
|
||||
break;
|
||||
@@ -429,6 +480,7 @@ bool Core_RequestCPUStep(CPUStepType type) {
|
||||
}
|
||||
BreakReason reason = type == CPUStepType::Into ? BreakReason::DebugStepInto : BreakReason::DebugStep;
|
||||
g_cpuStepQueue.push_back({ type, reason, 0 });
|
||||
Core_WakeIdleCPUThread();
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -657,6 +709,7 @@ void Core_Resume() {
|
||||
// Handle resuming from GE.
|
||||
if (coreState == CORE_STEPPING_GE) {
|
||||
coreState = CORE_RUNNING_GE;
|
||||
Core_WakeIdleCPUThread();
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -664,6 +717,7 @@ void Core_Resume() {
|
||||
Core_ResetException();
|
||||
coreState = CORE_RUNNING_CPU;
|
||||
g_breakReason = BreakReason::None;
|
||||
Core_WakeIdleCPUThread();
|
||||
System_Notify(SystemNotification::DEBUG_MODE_CHANGE);
|
||||
}
|
||||
|
||||
|
||||
@@ -231,6 +231,12 @@ CoreShutdownLock Core_LockAgainstShutdown();
|
||||
// Called from the CPU thread only - which is whatever thread NativeFrame() itself runs on.
|
||||
void Core_ProcessCPUQueue();
|
||||
|
||||
// While the CPU is stopped with nothing pending, the CPU thread blocks briefly rather than
|
||||
// spinning (see Core_IdleWaitWhileStepping() in Core.cpp). Call this after making state changes
|
||||
// that give it something to do, so it notices immediately instead of at the next timeout. Already
|
||||
// called by Core_RunOnCPUThread(), Core_RequestCPUStep() and Core_Resume(); free-threaded.
|
||||
void Core_WakeIdleCPUThread();
|
||||
|
||||
// Guards CPU-thread-owned debugger state (breakpoints, symbol map, registers, memory, etc.)
|
||||
// against concurrent unsynchronized reads from other threads' paint handlers.
|
||||
//
|
||||
|
||||
@@ -89,12 +89,12 @@ prints as it arrives; this only changes when the *next* line gets sent. A breakp
|
||||
trips would otherwise hang the script forever, so it gives up after `--sync-timeout` seconds
|
||||
(default 30), reports it, and makes the run exit non-zero.
|
||||
|
||||
Matching is by ticket, always. A raw JSON line (the only way to send nested parameters) gets a
|
||||
ticket assigned if it doesn't carry one, so it's waited for like any other line - previously it
|
||||
had none, `--sync` had nothing to match, and it skipped waiting entirely, which let the next
|
||||
line's response be read as this one's and quietly desynchronised the rest of the script. Raw lines
|
||||
are also rejected up front, rather than sent and left to fail somewhere downstream, if they aren't
|
||||
valid JSON, aren't an object, have no string `event`, or carry a `ticket` that isn't an integer.
|
||||
Matching is by ticket, always - `--sync` never waits for "whatever message arrives next", which is
|
||||
what used to quietly desynchronise a script. A raw JSON line (the only way to send nested
|
||||
parameters) is sent exactly as written, so it's waited for only if *you* gave it a `ticket`;
|
||||
without one there is nothing to match and `--sync` moves straight on to the next line. Raw lines
|
||||
are rejected up front, rather than sent and left to fail somewhere downstream, if they aren't valid
|
||||
JSON, aren't an object, have no string `event`, or carry a `ticket` that isn't an integer.
|
||||
|
||||
```bash
|
||||
(
|
||||
|
||||
Reference in new issue
Block a user