WIP: perf(host): three per-call costs on hot paths removed (own-thread lookups, a Signal with no waiter, switches read once) #156
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!156
Loading…
Reference in a new issue
No description provided.
Delete branch "klimovich008/Longshot:perf/os-small-calls"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Ported from Hookshot: https://github.com/NotRexed/Hookshot/pull/2708 (same commits, rebased onto Longshot main with the Longshot names).
Reviews and runs there:
Why
The game calls a few nn::os and host paths thousands of times a frame, and each call does work whose answer is already known. This PR removes that work and keeps its result, to save CPU time on the game's threads.
What changed
host/hos_thread.cthread_arg): GetThreadPriority, GetThreadId, ChangeThreadPriority and the others scanned every thread underthreads_lock. The game mostly asks about its own thread (GetThreadPriority(GetCurrentThread()), about 2,400 a frame in Hyrule Field).host/nn_os_sync.c): it returns at once when the condition variable's waiter count is 0, without the lock pair and the wake call.HOS_CV_SIGNAL_ALWAYS=1gives the old Signal.cv_count_in, a seq-cst add) before it takes the condition variable's lock, and so before it gives the guest mutex back (mutex_release_all). A signaller that holds the mutex therefore sees the waiter, and the waiter cannot be skipped. A signal that reads 0 was made before the waiter started to wait. The bookkeeping under the lock (mixed mutexes, hand-off reservations) uses its own count,cv_in, as before.gpu/vk/vk_menu.cpp,host/nn_fs.c):menu_vsync(RX_VK_FIFO/RX_VK_IMMEDIATE, twice a present) andHOS_FS_OVERLAY(every OpenFile) were read from the environment on every call. They hold for a whole run. Each is now read once and published atomically (HOS_FS_OVERLAYas one atomic pointer).RX_VK_DUMP_STAGESstays a per-readgetenv:vk_shader.cppis part of the program cache's stamp, so changing it would make every player's prepared shaders stale.RX_VK_TAAU_KEYSstays a per-dispatch read:tests/test_taau_env_lookup.pypins that.docs/development.md:HOS_CV_SIGNAL_ALWAYSin the switch reference.Flags and settings
HOS_CV_SIGNAL_ALWAYS(set)RX_VK_FIFO,RX_VK_IMMEDIATEandHOS_FS_OVERLAYkeep their meaning; they are now read once a run.Measurements
Measured on Hookshot, not yet on Longshot. Longshot's code in these files is the same as Hookshot's at the commits measured.
main:
46780503(2026-11-12, Hookshot, with #2602)this PR: Hookshot measure/os46 @ 3ca904eb: this PR's changes together with Hookshot #2709 (the sleep and spin changes, since merged there), over
46780503Baseline: main @
46780503(Hookshot). Not Longshot main against this PR: these runs were made on Hookshot before the move, and this PR was not measured alone on that test.Hardware: RX 9070 XT, AMD driver 26.10.2, Ryzen 7 5700X3D (16 logical processors), Windows 11
Scene: the standard Hyrule Field test as it was then: right stick held right from the load,
HOS_FPS_MAX=240, 2560x720 (32:9), 20 s window, warm cachesMethod: 6 alternating pairs a cell, medians; 8 and 6 threads: the game held to
0xFF/0x3FHow to reproduce:
claude-perf/std_test.py(the team's bench) with the hyrule scene; not a repo script46780503(Hookshot)46780503(the standard test, n=1) counted ~2,800 SignalConditionVariable calls a frame in the play window, none of them with a waiter: the case the early return skips.46780503. fps −0.27 (sd 0.68) at 2048x1152 and +0.58 (sd 1.10) with DLSS + frame generation at 165, both within noise; the frame there is limited by the replay on the Presentation thread. CPU per thread at 2048x1152: ModuleSystemWorker1-5 170 → 161 % of a core, all threads 574 → 562 %.46780503+ this PR, the prologue movie, 3 alternating pairs on one build): 530 / 346 / 356 withHOS_CV_SIGNAL_ALWAYS=1, 454 / 337 / 379 without. All six played to the end.Testing
Tested: this branch on Longshot main
39647e1a(then rebased onto1be180bb, which adds only release-script changes), Windows 11, Ryzen 7 5700X3D:tests/test_thread_arg_self.py(new):thread_argfrom the source, over own and other ThreadTypes. Broken variants must fail: every call scanning, and the caller's record returned for any ThreadType.tests/test_cv_signal_early.py(new): Signal over waiter counts, with and withoutHOS_CV_SIGNAL_ALWAYS. Broken variants must fail: the early return taken even with the switch, the early return reading the wrong count, a waiter that does not count itself in. Structural checks: in both waitscv_count_incomes before the condition variable's lock, andcv_enterbeforemutex_release_all.tests/test_cv_signal_race.py(new): the real Signal, Wait and TimedWait on threads (pthreads for the Win32 calls).cv_enterwhile a thread that does not own the mutex signals. It must wake: 20 rounds each wait, with and withoutHOS_CV_SIGNAL_ALWAYS=1.tests/test_vsync_row.py: one process per switch setting now (the value is read once), with a negative control.python -I -B tests/run_all.py(LONGSHOT_MINGW= llvm-mingw 20261104): 468 passed, 69 skipped, 4 failed. The 4 fail on main on this PC too:nx2/test_nup_paths.py,tests/test_check_third_party.py,tests/test_setup_runtime.py,tests/test_release.py.host/*.c(llvm-mingw clang, against a build's generated runtime headers): the changed files compile. 5 files stop at library headers this PC lacks (dav1d, zstd, opus, libavcodec), the same on main.Not tested: this PR alone on the standard test; a Longshot build in the game; other scenes; Linux with the Signal fix (f5a854f3); macOS.
Needs other testers:
HOS_CV_SIGNAL_ALWAYS=1against unset on one build, counting skipped pictures, expecting no consistent gap between the two (request to open)Screenshots
Nothing on screen.
Risk and rollback
A missed wake would hang a thread waiting on a condition variable. The ordering above and
tests/test_cv_signal_race.pycover it;HOS_CV_SIGNAL_ALWAYS=1turns the early return off. The own-thread lookup and the switch reads give the same results as before; reverting the commits is enough.Reviewer notes
Merge order with #157 (BroadcastConditionVariable): both change
host/nn_os_sync.cnext to Signal and merge without a textual conflict. Whichever lands second needs a short follow-up:cv_in - cv_owed, the waiters under the condition variable's lock, instead ofcv_waiting;cv_in;The follow-up is ready, applies to both orders on Longshot, and will be pushed right after the first of the two merges.
Issues
None.
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).46780503only; a Longshot measurement of this PR alone is still to do, so this stays WIP.)third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened. (tests/test_vsync_row.py: one process per switch setting, with a negative control.)🤖 Generated with Claude Code
Read-only review of the PR at
7da97528against main@originfbd42024. I read the code and did not run any tests.What's good
thread_arg). This is safe.curis the calling thread's own_Thread_localrecord. It is set inthread_mainandhos_boot_main_threadand is never freed while that thread runs, so a ThreadType or handle being reused elsewhere can't make it stale. When the caller's ownguest_ttmatches, the old scan would have returned the same record. Threads the host didn't create havecur == NULL, and thecur &&guard sends them to the scan as before.hos_thread_adopt_guestupdatescur->guest_ttin place, so the main thread's adopted ThreadType still matches. No new TLS is added:curwas already_Thread_localin the exe, not in the renderer DLL.cv_count_inis a seq-cst add, made before the waiter takescv->lkand so before it readsgen.mutex_release_all's seq-cst exchange, so it always sees the count.genread. The old Signal could have run fully at that point with the same result, so the skip loses nothing.cv_count_in(Wait and TimedWait) is matched by the__atomic_subincv_leaveon every path, timeout included.cv_inunder the lock.61fb0a48(waiter counted beforecv->lk). I couldn't open the Hookshot PR to diff f5a854f3 byte for byte, but the content matches its description.event_signalstill setssignaledunconditionally.longshot_env.txtis applied at boot, before the game or the first present (load_longshot_env).env_release/env_holdsonly track rows and never write the environment.live_ontable, and none of them isRX_VK_FIFO,RX_VK_IMMEDIATE,HOS_FS_OVERLAYorHOS_CV_SIGNAL_ALWAYS.menu_vsyncuses a C++ magic static (no-fno-threadsafe-staticsanywhere), and the C ones are atomic loads and stores of an idempotent value.__atomic_*and_Thread_localwork on llvm-mingw and on Linux clang/gcc. Neither is new to these files.Must change
1be180bb, and main@origin is 38 commits ahead, including changes tohost/hos_thread.c,gpu/vk/vk_menu.cppanddocs/development.md. I see no overlapping hunks, so it should rebase cleanly. Rebase, reruntests/run_all.py, then tick the box.bd2b56d3("perf(host): three per-call costs on hot paths removed ... (#2708)") has the same code and the same four tests as this PR, line for line. The only difference is thedocs/development.mdrow, which 209 doesn't have. Agree with 209's author on which PR carries it:bd2b56d3.61fb0a48the first statement is wrong and the second can no longer happen. This comment is the safety argument for the riskiest change in the PR, so it should match the code.Should change
cv_reserve(now oncv_in) into a fixtureObjwithoutcv_in. CONTRIBUTING expects the check to be updated in the same PR.cv->cv_waitingwith a plain load undercv->lk, while the waits now change it atomically outside that lock. That is a data race, thoughcv_reserve'scv_inguard keeps the result harmless.cv_in, as the description says.HOS_FS_OVERLAY(possible on Linux only) madealt"/<path>", a lookup at the filesystem root. The new behaviour is better; the comment should say it changed.@ -378,0 +380,4 @@const char *ov = __atomic_load_n(&overlay, __ATOMIC_RELAXED);if (ov && !*ov) {ov = getenv("HOS_FS_OVERLAY");if (ov && !*ov) ov = NULL; /* (set but empty: no overlay, as before) */Nit: "as before" isn't quite right. Before, an empty
HOS_FS_OVERLAY(possible on Linux; Windows_putenv("X=")unsets it) madealt"/<path>", a lookup at the filesystem root. Turning that off is better; the comment should say the behaviour changed.@ -461,3 +469,3 @@/* a signal (cv->lk held): the mutex reserved for the waiter it wakes */static void cv_reserve(Obj *cv) {if (!handoff_on() || cv->cv_mixed || !cv->cv_mutex || cv->cv_waiting <= cv->cv_owed || cv->cv_owed >= 8) return;if (!handoff_on() || cv->cv_mixed || !cv->cv_mutex || cv->cv_in <= cv->cv_owed || cv->cv_owed >= 8) return;With #157 this needs fixing in the PR that merges second, not in a follow-up:
cv->cv_waiting - cv->cv_owedwith a plain read undercv->lk, while the waits now changecv_waitingatomically outside that lock. That is a data race, though harmless here because this guard stops atcv_in.Objhas nocv_in, so its test stops compiling oncecv_reservereads it.Broadcast should count from
cv_in, and 157's fixture needscv_in.@ -578,1 +586,4 @@HLE(_ZN2nn2os25FinalizeConditionVariableEPNS0_21ConditionVariableTypeE) { drop(ARG(0)); }/* A signal with no waiter returns at once (~2,500 signals a frame in Hyrule: each took cv->lk and made a wake call, awaiter there or not), as glibc's pthread_cond_signal does, and as Horizon's user mode does when the key reads 0. Nowaiter that can still miss its predicate's change is missed: a waiter counts itself in (cv_enter) under cv->lk beforeThis comment describes the code as it was before
61fb0a48. The waiter now counts itself in withcv_count_in, beforecv->lk, not incv_enterunder it. A signaller that doesn't hold the mutex can no longer miss a waiter between its gen snapshot and its sleep: closing that gap was the point of the fix. Please restate the argument as the code has it now. A signal that reads 0 is ordered before the waiter's count-in, and so before itsgenread, so the skipped signal is one the old Signal could also have made before the wait began.I rebased your eight commits onto main @
fbd42024(no conflicts) and fixed the two comments from my review in one commit on top. It's pushed as222f568e, and your commits are unchanged apart from the rebase.Two corrections to my review:
My commit, which changes comments only:
The Signal comment in
host/nn_os_sync.cnow describes the code as it is after61fb0a48:cv_count_in(sequentially consistent) while it still holds the guest mutex, before it takescv->lkand readsgen;cv_leaveon every way out of Wait and TimedWait, a timeout included.I checked the order against both waits: count-in,
cv->lk,mutex_release_all, thencv_leave.host/nn_fs.c's emptyHOS_FS_OVERLAYcomment now says this changed: before, an empty value, which only Linux can set, made every file look up at the filesystem root.No tests were run here. The checks that cut these functions don't match on comment text. Still open from the review: a Longshot A/B (the Measured box), and whichever of this PR and #157 lands second updating the other's fixture.
7da97528a6222f568e66Code review only; not built or run.
Edited: the Kokiri profile quoted below drew a 5:4 picture, 1800×1440, not 2048×1152 as I first wrote: the PC's display mode had changed that hour. At the usual 16:9 picture the same route runs at about 74 fps. A rerun of the profile there is queued, and I'll update the numbers if they move.
Thanks for porting this over. HumanBean's review covers the ordering argument and the rest, and I found nothing new on correctness. What I checked on my side:
cv_count_inandcv_leavepair up on the one way out ofWaitConditionVariableandTimedWaitConditionVariable, the timeout included.curnever has a zeroguest_tt(the main thread's comes fromhos_sys_alloc), so the own-thread test can't match a null handle.getenv's pointer is safe.What I can add is where these three costs sit in Longshot. This is from a sampled profile of Kokiri Forest on Longshot
1ef93e79(i7-13700K, RTX 5090, Windows, 1800×1440 (5:4), uncapped, about 79 fps; all threads, named with the build's linker maps):All of it is on the job workers. In that scene the frame is paced by the main thread's hand-offs to its jobs: the main thread waits for them 51 % of its time, about 6.4 ms a frame. Those hand-offs are light events and light message queues (
WaitLightEvent/SignalLightEvent,Send/ReceiveLightMessageQueue), which this PR doesn't touch. So the only way it can shorten that wait is through the ~0.15 ms a frame of worker CPU it frees. On a 24-thread PC I'd expect no change in fps. That fits the Hookshot version of this change (#2708 there), which measured flat on the same PC (−0.27 ± 0.68 fps in Kokiri).For the Longshot A/B, two suggestions:
No objection from me once HumanBean's points are settled.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.