perf(boot): four modules off the boot-critical path -- POPSLoader-shape lazy tier-0 - #252
Closed
NathanNeurotic wants to merge 2 commits into
Closed
perf(boot): four modules off the boot-critical path -- POPSLoader-shape lazy tier-0#252NathanNeurotic wants to merge 2 commits into
NathanNeurotic wants to merge 2 commits into
Conversation
…r-shape lazy tier-0 (maintainer directive) Product of the POPSLoader gap analysis (wf_2662799e-a7d, both tracks source-verified). THE HEADLINE FINDING: the directed shape is ALREADY substantially ours -- config home is cwd-driven (configInit(gBootDir)), sysReset loads ZERO storage transports, and mass boots already resolve their backing device via USBMASS_IOCTL_GET_DRIVERNAME (bdmReadDeviceIdentity in the resolver scan) rather than assuming USB. Reading POPSLoader's source also corrected two assumptions: it IOP-RESETS at boot (Makefile RESET_IOP=1 -- its speed is not inherited mounts), and its "fast settings read" is a DODGE, not a solution: it reads settings from mc0:/POPSTARTER/.pldrs (always-up device) and embeds every other asset in the ELF -- it never solved cwd-settings-over-arbitrary-devices. Ours does. THE GENUINE TRANSFERABLE WIN (this commit): sysReset serialized four module DMAs onto the boot-critical path of EVERY flavor -- including the fast mc/mmce boots -- for modules nothing needs before the menu: libsd + audsrv (audio), isofs + genvmc (launch prep). Now: - libsd + audsrv load at the top of deferredAudioInit (their consumers' existing deferred seam; audioInit binds audsrv right below). - isofs + genvmc load via the new idempotent sysLoadLaunchModules(): from deferredInit on the IO worker for the GUI path (resident long before the menu is interactive -- never AT launch, per the Delta-4 no-module-loads-in-the-launch-sequence doctrine), and SYNCHRONOUSLY on the autolaunch fork, which skips deferredInit and needs both before its launch prep. NOT changed, deliberately (from the ranked candidate list): the untyped-mass ATA escalation stays (post-#250 its failure is retryable; residual cost is corner-case latency only); mmceInit stays synchronous (#152 THM/LNG starvation risk); menu-before-first-scan ordering stays (load-bearing for the #132 boot-step localizer; explicitly not to be stacked with the live HW triage). One candidate is flagged for a MAINTAINER UX DECISION in the PR: bdmLoadBlockDeviceModules loads every gEnable*'d transport at boot regardless of Manual/Auto -- gating Manual transports to tab entry would change what "Manual" means for tab visibility/hotplug. Builds clean (make opl.elf, exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughModule loading is moved out of ChangesDeferred module loading
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant deferredInit
participant deferredAudioInit
participant main
participant ModuleLoaders
deferredInit->>ModuleLoaders: load launch modules
deferredAudioInit->>ModuleLoaders: load audio modules
deferredAudioInit->>deferredAudioInit: initialize audio and sound effects
main->>ModuleLoaders: load launch modules for autolaunch
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/system.c`:
- Around line 298-302: Update the module-loading helpers around
sysLoadModuleBuffer calls to capture and validate both libsd_irx and audsrv_irx
load results, returning failure to their callers when either load fails. Set
loaded = 1 only after both modules succeed, and ensure the src/opl.c call sites
propagate or handle the helper failure so deferred or autolaunch attempts can
retry.
- Around line 295-297: Update sysReset() to clear the static loaded latches used
by sysLoadAudioModules() and sysLoadLaunchModules(), alongside
g_sysLoadedModBuffer. Ensure subsequent calls after a reset reload libsd/audsrv
and isofs/genvmc instead of returning early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77abdd06-6f16-4fda-8994-0a79dc43047c
📒 Files selected for processing (3)
include/system.hsrc/opl.csrc/system.c
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: build-debug (DTL_T10000=1, :v20250725-2)
- GitHub Check: build-debug (iopcore_ppctty_debug, :v20250725-2)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=0)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=1, PADEMU=0)
- GitHub Check: build-debug-ps2dev-latest (iopcore_debug)
- GitHub Check: build-debug (ingame_ppctty_debug, :v20250725-2)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=1, PADEMU=1)
- GitHub Check: build-debug (iopcore_debug, :v20250725-2)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-debug-ps2dev-latest (DTL_T10000=1)
- GitHub Check: build
- GitHub Check: build-debug (eesio_debug, :v20250725-2)
- GitHub Check: build-debug-ps2dev-latest (ingame_debug)
- GitHub Check: build-debug-ps2dev-latest (eesio_debug)
- GitHub Check: build-ps2dev-latest
- GitHub Check: build-debug-ps2dev-latest (iopcore_ppctty_debug)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=1)
🧰 Additional context used
🪛 Cppcheck (2.21.0)
src/system.c
[style] 293-293: The function 'sysLoadAudioModules' is never used.
(unusedFunction)
[style] 305-305: The function 'sysLoadLaunchModules' is never used.
(unusedFunction)
🔇 Additional comments (3)
include/system.h (1)
20-22: LGTM!src/system.c (1)
261-267: LGTM!src/opl.c (1)
3003-3007: LGTM!Also applies to: 3058-3060, 3353-3356
Comment on lines
+298
to
+302
| LOG("[LIBSD]:\n"); | ||
| sysLoadModuleBuffer(&libsd_irx, size_libsd_irx, 0, NULL); | ||
| LOG("[AUDSRV]:\n"); | ||
| sysLoadModuleBuffer(&audsrv_irx, size_audsrv_irx, 0, NULL); | ||
| loaded = 1; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Only mark a module group loaded after every member succeeds.
sysLoadModuleBuffer() can return an error, but both helpers ignore the results and set loaded = 1 unconditionally. If either module fails, later deferred or autolaunch calls become no-ops, so the missing module cannot recover. Capture/check both return values, propagate failure to the callers, and set the guard only after both loads succeed.
As per the supplied src/system.c loader implementation and src/opl.c call sites.
Also applies to: 310-314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/system.c` around lines 298 - 302, Update the module-loading helpers
around sysLoadModuleBuffer calls to capture and validate both libsd_irx and
audsrv_irx load results, returning failure to their callers when either load
fails. Set loaded = 1 only after both modules succeed, and ensure the src/opl.c
call sites propagate or handle the helper failure so deferred or autolaunch
attempts can retry.
- The one-shot latches now set ONLY when every member load succeeds (CodeRabbit major -- real, and embarrassingly the exact latch-on-failure class the atad/mmce fixes just eradicated). - sysReset() clears both latches: after an IOP reboot nothing is resident, so the deferred loaders must reload on their next call (CodeRabbit minor -- real for any future in-process reset). Builds clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/system.c`:
- Around line 302-323: Change sysLoadAudioModules() and sysLoadLaunchModules()
to return a success/error status based on both module-load results, update their
declarations in include/system.h, and modify deferredAudioInit() plus the
launch-related callers in src/opl.c to check the status and stop or report
before proceeding with dependent initialization or autolaunch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 110b40a6-92b4-401c-89a4-afa89c339d03
📒 Files selected for processing (1)
src/system.c
📜 Review details
⏰ Context from checks skipped due to timeout. (20)
- GitHub Check: check-format
- GitHub Check: build-debug (iopcore_debug, :v20250725-2)
- GitHub Check: build-debug (DTL_T10000=1, :v20250725-2)
- GitHub Check: build-debug (eesio_debug, :v20250725-2)
- GitHub Check: build-debug (ingame_ppctty_debug, :v20250725-2)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=1, PADEMU=1)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-debug-ps2dev-latest (ingame_debug)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build-debug-ps2dev-latest (eesio_debug)
- GitHub Check: build-debug-ps2dev-latest (DTL_T10000=1)
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=0)
- GitHub Check: build-variants (EXTRA_FEATURES=1, PADEMU=1)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=1)
- GitHub Check: build-debug-ps2dev-latest (ingame_ppctty_debug)
- GitHub Check: build-variants-ps2dev-latest (EXTRA_FEATURES=1, PADEMU=0)
- GitHub Check: build-variants (EXTRA_FEATURES=0, PADEMU=0)
- GitHub Check: build-ps2dev-latest
- GitHub Check: build-lang
- GitHub Check: build
🧰 Additional context used
🪛 Cppcheck (2.21.0)
src/system.c
[style] 302-302: The function 'sysLoadAudioModules' is never used.
(unusedFunction)
[style] 314-314: The function 'sysLoadLaunchModules' is never used.
(unusedFunction)
🔇 Additional comments (2)
src/system.c (2)
65-67: LGTM!Also applies to: 246-249
268-274: LGTM!Also applies to: 296-301
Comment on lines
+302
to
+323
| void sysLoadAudioModules(void) | ||
| { | ||
| if (sysAudioModsLoaded) | ||
| return; | ||
| LOG("[LIBSD]:\n"); | ||
| int r0 = sysLoadModuleBuffer(&libsd_irx, size_libsd_irx, 0, NULL); | ||
| LOG("[AUDSRV]:\n"); | ||
| int r1 = sysLoadModuleBuffer(&audsrv_irx, size_audsrv_irx, 0, NULL); | ||
| if (r0 >= 0 && r1 >= 0) | ||
| sysAudioModsLoaded = 1; | ||
| } | ||
|
|
||
| void sysLoadLaunchModules(void) | ||
| { | ||
| if (sysLaunchModsLoaded) | ||
| return; | ||
| LOG("[ISOFS]:\n"); | ||
| int r0 = sysLoadModuleBuffer(&isofs_irx, size_isofs_irx, 0, NULL); | ||
| LOG("[GENVMC]:\n"); | ||
| int r1 = sysLoadModuleBuffer(&genvmc_irx, size_genvmc_irx, 0, NULL); | ||
| if (r0 >= 0 && r1 >= 0) | ||
| sysLaunchModsLoaded = 1; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate module-load failures to the callers.
The latch handling is now correct, but both helpers still discard failure status because they return void. deferredAudioInit() proceeds to audioInit(), and the launch paths proceed after sysLoadLaunchModules() even when either required module failed to load. Return an error and make the callers stop, report, or otherwise handle the dependent operation; otherwise the autolaunch path in particular may continue without a retry.
Suggested shape
-void sysLoadAudioModules(void)
+int sysLoadAudioModules(void)
{
if (sysAudioModsLoaded)
- return;
+ return 0;
...
- if (r0 >= 0 && r1 >= 0)
+ if (r0 >= 0 && r1 >= 0) {
sysAudioModsLoaded = 1;
+ return 0;
+ }
+ return -1;
}Apply the same contract to sysLoadLaunchModules(), update include/system.h, and handle the return values at the src/opl.c call sites.
🧰 Tools
🪛 Cppcheck (2.21.0)
[style] 302-302: The function 'sysLoadAudioModules' is never used.
(unusedFunction)
[style] 314-314: The function 'sysLoadLaunchModules' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/system.c` around lines 302 - 323, Change sysLoadAudioModules() and
sysLoadLaunchModules() to return a success/error status based on both
module-load results, update their declarations in include/system.h, and modify
deferredAudioInit() plus the launch-related callers in src/opl.c to check the
status and stop or report before proceeding with dependent initialization or
autolaunch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap analysis verdict first (both sides read from source)
The shape you pointed at is already substantially ours: config home is cwd-driven,
sysResetloads zero storage transports, and mass boots already resolve their backing device via the driver-name ioctl in the resolver scan — we probe-and-confirm, not assume-USB. Two honest corrections from reading POPSLoader's actual source: it IOP-resets at boot (its speed is not inherited mounts — fully transferable comparison), and its "fast settings read" is a dodge, not a solution — it always reads settings frommc0:and embeds every other asset in the ELF; it never solved reading settings from an arbitrary cwd device. Ours does. Also flagged from their source, not copied: a fixedsleep(2)racing HDD spin-up (the exact hazard class behind our APA death) and a boot-device-lock ordering bug that makes their marker files a silent no-op.What ships here — the one clean transferable win
sysResetserialized four module DMAs onto the boot-critical path of every flavor for modules nothing needs before the menu:libsd+audsrvnow load indeferredAudioInit(their consumers' existing seam),isofs+genvmcvia an idempotent one-shot fromdeferredIniton the IO worker — resident long before the menu is interactive, never at launch time (Δ4 doctrine) — with a synchronous call on the autolaunch fork, which skipsdeferredInitand needs both.Deliberately not changed (ranked and rejected with reasons)
mmceInitsync: stays — deferring risks the [ISSUE]: Riptopl can't find themes in MMCE #152 THM/LNG starvation.One UX decision for you
bdmLoadBlockDeviceModulesloads every enabled transport at boot regardless of Manual vs Auto. Gating Manual transports to tab entry would be lazier — but a Manual page's tab could then never auto-appear on device presence (the driver that detects presence wouldn't be loaded). Current behavior: Manual gates only the scan, not the driver. Say the word if you want Manual to mean "nothing loads until I enter the tab", and I'll implement it with that explicit semantic.Builds clean.
🤖 Generated with Claude Code
Summary by CodeRabbit