Restrict FFP fastpath shim to external storage volumes - #1843
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe X server screen now enables FFP environment variables for ChangesFFP environment configuration
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The change narrows fastpath-shim use for custom games on internal storage while preserving existing behavior elsewhere; no actionable merge-blocking risk remains after normal checks. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
utkarshdalal
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:3842">
P2: The new path-only gate re-enables the FFP shim for custom games whose drive A happens to live under /Android/data/ on external storage, which contradicts the PR goal ("custom games no longer get gating variables automatically") and its own trade-off note. Custom game A: paths come from user-selected folders (SteamService customGameManualFolders), so a custom game placed under /storage/.../Android/data/ now matches `contains("/Android/data/")` and receives FFP_ENABLE/FFP_MARKERS — the scenario the description's trade-off implies should not get the boot-speedup shim. Keep the custom-game exclusion in addition to the path check.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .firstOrNull { it[0] == "A" }?.let { File(it[1]).canonicalFile.path } | ||
| }.getOrNull() ?: "" | ||
| if (ffpGameDir.startsWith("/storage/")) { | ||
| if (ffpGameDir.startsWith("/storage/") && ffpGameDir.contains("/Android/data/")) { |
There was a problem hiding this comment.
P2: The new path-only gate re-enables the FFP shim for custom games whose drive A happens to live under /Android/data/ on external storage, which contradicts the PR goal ("custom games no longer get gating variables automatically") and its own trade-off note. Custom game A: paths come from user-selected folders (SteamService customGameManualFolders), so a custom game placed under /storage/.../Android/data/ now matches contains("/Android/data/") and receives FFP_ENABLE/FFP_MARKERS — the scenario the description's trade-off implies should not get the boot-speedup shim. Keep the custom-game exclusion in addition to the path check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt, line 3842:
<comment>The new path-only gate re-enables the FFP shim for custom games whose drive A happens to live under /Android/data/ on external storage, which contradicts the PR goal ("custom games no longer get gating variables automatically") and its own trade-off note. Custom game A: paths come from user-selected folders (SteamService customGameManualFolders), so a custom game placed under /storage/.../Android/data/ now matches `contains("/Android/data/")` and receives FFP_ENABLE/FFP_MARKERS — the scenario the description's trade-off implies should not get the boot-speedup shim. Keep the custom-game exclusion in addition to the path check.</comment>
<file context>
@@ -3839,7 +3839,7 @@ private fun setupXEnvironment(
.firstOrNull { it[0] == "A" }?.let { File(it[1]).canonicalFile.path }
}.getOrNull() ?: ""
- if (ffpGameDir.startsWith("/storage/") && gameSource != GameSource.CUSTOM_GAME) {
+ if (ffpGameDir.startsWith("/storage/") && ffpGameDir.contains("/Android/data/")) {
envVars.put("FFP_ENABLE", "1")
envVars.put("FFP_MARKERS", "/steamapps/common/;/dosdevices/a:")
</file context>
| if (ffpGameDir.startsWith("/storage/") && ffpGameDir.contains("/Android/data/")) { | |
| if (ffpGameDir.startsWith("/storage/") && ffpGameDir.contains("/Android/data/") && gameSource != GameSource.CUSTOM_GAME) { |
|
Closing — the /storage/ gate on master is the correct, device-validated design. The shim's win was measured on public storage paths too, so excluding custom games (or narrowing to Android/data) would regress their boot times. The custom-game perf reports need a different explanation. |
utkarshdalal
changed the title
utkarshdalal
changed the title
1 participant
Description
1.2.0's store-agnostic FFP gate (bad4d39) enables the fastpath shim for any game whose A: drive is under /storage/ — including internal shared storage (/storage/emulated/), where the shim's benefit was never measured. A user's game (custom game imported as a Steam game, installed under /storage/emulated/0/Download/Games/steamapps/common/) refused to boot on 1.2.0: the environment came up but the guest process died before wine printed a single line. Setting FFP_DISABLE=1 in the container env vars fixed it, confirming the shim kills the guest at process bootstrap on that combination (bionic container, proton-11.0-1-arm64ec, FEXCore 2604). The same gate is the prime suspect for the wider custom-game perf regression reports on internal storage.
This restores the original intent of the gate: FFP only activates when the game tree is on a non-emulated /storage volume (SD cards / USB), where its win is device-validated (BL2 6.5min → seconds). Internal-storage games — where metadata ops are already dcache-fast — no longer get the shim. Game source no longer matters, so custom games imported as Steam games are covered too. FFP_DISABLE=1 remains available as a per-container kill switch.
Follow-up (not this PR): reproduce and fix the shim's process-bootstrap crash with proton-11-arm64ec/FEX so external-storage installs of those containers don't hit it.
Recording
Not applicable — env var gating change, no UI.
Type of Change
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.