Repository navigation
Place the frame thread on the prime cluster and keep spinners off it - #2370
Closed
utkarshdalal wants to merge 3 commits into
Closed
utkarshdalal wants to merge 3 commits into
utkarshdalal wants to merge 3 commits into
Conversation
Contributor
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt">
<violation number="1" location="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt:333">
P2: Disabling power control leaves existing thread masks active until the next placement tick, and a quick re-enable can prevent release altogether. Release placement synchronously from the power-control toggle path instead of relying on polling.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt:346">
P2: An empty scan skips role cleanup, so previously placed TIDs remain tracked after all tracked processes disappear or a scan fails. Release the existing placements when the scan is empty before returning.</violation>
<violation number="3" location="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt:389">
P2: Placement state identifies threads by TID alone, so a reused TID can inherit stale placement and restoration state from a previous process. Key the state by `(pid, tid)` and release mismatched entries before applying the new role.</violation>
</file>
<file name="app/src/main/java/app/gamenative/powercontrol/PowerManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/powercontrol/PowerManager.kt:1179">
P2: Disabling power control can race with a tick that already captured active clusters, allowing `applyWindow()` to place threads after the toggle is off. Recheck the toggle under the placement lock or release placements synchronously when disabling power control.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| val desired = desiredPlacements(threads, roles, plan) | ||
| (placed.keys - desired.keys).forEach { tid -> releaseThread(tid, "role lost", setAffinity, gamePin) } | ||
| desired.forEach { (tid, p) -> | ||
| val prev = placed[tid] |
Contributor
There was a problem hiding this comment.
P2: Placement state identifies threads by TID alone, so a reused TID can inherit stale placement and restoration state from a previous process. Key the state by (pid, tid) and release mismatched entries before applying the new role.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt, line 387:
<comment>Placement state identifies threads by TID alone, so a reused TID can inherit stale placement and restoration state from a previous process. Key the state by `(pid, tid)` and release mismatched entries before applying the new role.</comment>
<file context>
@@ -0,0 +1,548 @@
+ val desired = desiredPlacements(threads, roles, plan)
+ (placed.keys - desired.keys).forEach { tid -> releaseThread(tid, "role lost", setAffinity, gamePin) }
+ desired.forEach { (tid, p) ->
+ val prev = placed[tid]
+ if (prev == null && p.kind != ProcessKind.GAME) {
+ restoreCores[tid] = readAllowed(p.pid, tid) ?: plan.allCores
</file context>
| private fun tick(scanner: Scanner) { | ||
| val gamePin = PowerManager.threadPlacementGamePin() | ||
| val clusters = PowerManager.threadPlacementClusters() | ||
| if (clusters == null) { |
Contributor
There was a problem hiding this comment.
P2: Disabling power control leaves existing thread masks active until the next placement tick, and a quick re-enable can prevent release altogether. Release placement synchronously from the power-control toggle path instead of relying on polling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt, line 333:
<comment>Disabling power control leaves existing thread masks active until the next placement tick, and a quick re-enable can prevent release altogether. Release placement synchronously from the power-control toggle path instead of relying on polling.</comment>
<file context>
@@ -0,0 +1,548 @@
+ private fun tick(scanner: Scanner) {
+ val gamePin = PowerManager.threadPlacementGamePin()
+ val clusters = PowerManager.threadPlacementClusters()
+ if (clusters == null) {
+ synchronized(this) {
+ if (!active()) return
</file context>
| * [FrameThreadPlacement] moves the game's own threads and places nothing with the toggle off. | ||
| */ | ||
| internal fun threadPlacementClusters(): List<CoreCluster>? = | ||
| if (::currentProfile.isInitialized && isProfilePowerControlEnabled()) placementClusters() else null |
Contributor
There was a problem hiding this comment.
P2: Disabling power control can race with a tick that already captured active clusters, allowing applyWindow() to place threads after the toggle is off. Recheck the toggle under the placement lock or release placements synchronously when disabling power control.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/PowerManager.kt, line 1179:
<comment>Disabling power control can race with a tick that already captured active clusters, allowing `applyWindow()` to place threads after the toggle is off. Recheck the toggle under the placement lock or release placements synchronously when disabling power control.</comment>
<file context>
@@ -1160,6 +1171,19 @@ object PowerManager {
+ * [FrameThreadPlacement] moves the game's own threads and places nothing with the toggle off.
+ */
+ internal fun threadPlacementClusters(): List<CoreCluster>? =
+ if (::currentProfile.isInitialized && isProfilePowerControlEnabled()) placementClusters() else null
+
+ /** Cores the game is held to: the power control pin, else the container's CPU list, else none. */
</file context>
| return | ||
| } | ||
| val threads = scanner.scan() | ||
| if (threads.isEmpty()) return |
Contributor
There was a problem hiding this comment.
P2: An empty scan skips role cleanup, so previously placed TIDs remain tracked after all tracked processes disappear or a scan fails. Release the existing placements when the scan is empty before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt, line 346:
<comment>An empty scan skips role cleanup, so previously placed TIDs remain tracked after all tracked processes disappear or a scan fails. Release the existing placements when the scan is empty before returning.</comment>
<file context>
@@ -0,0 +1,548 @@
+ return
+ }
+ val threads = scanner.scan()
+ if (threads.isEmpty()) return
+ val plan = placementPlan(clusters, gamePin)
+ val clamp = PowerManager.latestTunerCaps()?.primeKhz
</file context>
Suggested change
| if (threads.isEmpty()) return | |
| + if (threads.isEmpty()) { | |
| + synchronized(this) { | |
| + if (active()) release("no threads", TaskAffinity::set, gamePin) | |
| + } | |
| + return | |
| + } |
Contributor
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/PrefManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/PrefManager.kt:1982">
P2: This adds the advertised preference but does not expose it in the app, so users have no way to disable frame-thread placement through `frameThreadPlacement`; add a performance-settings switch and persist its value through this property.</violation>
</file>
<file name="app/src/main/java/app/gamenative/powercontrol/PowerManager.kt">
<violation number="1" location="app/src/main/java/app/gamenative/powercontrol/PowerManager.kt:1179">
P2: This gate keeps frame placement active across `PowerManager.pause()`: backgrounding stops the driver, but the profile flag remains enabled, so the 2-second loop continues moving game threads. Stop placement when pausing and restart it on resume, or gate it on an actual running state.</violation>
</file>
<file name="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt">
<violation number="1" location="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt:203">
P2: Affinity state is keyed only by TID, but this scanner tracks both `pid` and `tid`; after a task exits, Linux may reuse that TID for a different process during the session. `desiredPlacements` then skips taking the new task's original mask, and `releaseThread` can restore the old task's mask to the new task; key state by `(pid, tid)` or invalidate it when the PID changes.
(Based on your team's feedback about preserving lifecycle identity.)</violation>
<violation number="2" location="app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt:346">
P2: An empty scan currently leaves every prior placement active instead of treating the tracked-thread set as empty. Release the entries in `placed` before returning when the scan has no threads, otherwise a transient `/proc` read failure can keep a live thread on a stale affinity mask.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| set(value) { setPref(SHARED_SHADER_CACHE, value) } | ||
|
|
||
| private val FRAME_THREAD_PLACEMENT = booleanPreferencesKey("frame_thread_placement") | ||
| var frameThreadPlacement: Boolean |
Contributor
There was a problem hiding this comment.
P2: This adds the advertised preference but does not expose it in the app, so users have no way to disable frame-thread placement through frameThreadPlacement; add a performance-settings switch and persist its value through this property.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/PrefManager.kt, line 1982:
<comment>This adds the advertised preference but does not expose it in the app, so users have no way to disable frame-thread placement through `frameThreadPlacement`; add a performance-settings switch and persist its value through this property.</comment>
<file context>
@@ -1977,4 +1977,9 @@ object PrefManager {
set(value) { setPref(SHARED_SHADER_CACHE, value) }
+
+ private val FRAME_THREAD_PLACEMENT = booleanPreferencesKey("frame_thread_placement")
+ var frameThreadPlacement: Boolean
+ get() = getPref(FRAME_THREAD_PLACEMENT, true)
+ set(value) { setPref(FRAME_THREAD_PLACEMENT, value) }
</file context>
| * [FrameThreadPlacement] moves the game's own threads and places nothing with the toggle off. | ||
| */ | ||
| internal fun threadPlacementClusters(): List<CoreCluster>? = | ||
| if (::currentProfile.isInitialized && isProfilePowerControlEnabled()) placementClusters() else null |
Contributor
There was a problem hiding this comment.
P2: This gate keeps frame placement active across PowerManager.pause(): backgrounding stops the driver, but the profile flag remains enabled, so the 2-second loop continues moving game threads. Stop placement when pausing and restart it on resume, or gate it on an actual running state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/PowerManager.kt, line 1179:
<comment>This gate keeps frame placement active across `PowerManager.pause()`: backgrounding stops the driver, but the profile flag remains enabled, so the 2-second loop continues moving game threads. Stop placement when pausing and restart it on resume, or gate it on an actual running state.</comment>
<file context>
@@ -1160,6 +1171,19 @@ object PowerManager {
+ * [FrameThreadPlacement] moves the game's own threads and places nothing with the toggle off.
+ */
+ internal fun threadPlacementClusters(): List<CoreCluster>? =
+ if (::currentProfile.isInitialized && isProfilePowerControlEnabled()) placementClusters() else null
+
+ /** Cores the game is held to: the power control pin, else the container's CPU list, else none. */
</file context>
| return | ||
| } | ||
| val threads = scanner.scan() | ||
| if (threads.isEmpty()) return |
Contributor
There was a problem hiding this comment.
P2: An empty scan currently leaves every prior placement active instead of treating the tracked-thread set as empty. Release the entries in placed before returning when the scan has no threads, otherwise a transient /proc read failure can keep a live thread on a stale affinity mask.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt, line 346:
<comment>An empty scan currently leaves every prior placement active instead of treating the tracked-thread set as empty. Release the entries in `placed` before returning when the scan has no threads, otherwise a transient `/proc` read failure can keep a live thread on a stale affinity mask.</comment>
<file context>
@@ -0,0 +1,551 @@
+ return
+ }
+ val threads = scanner.scan()
+ if (threads.isEmpty()) return
+ val plan = placementPlan(clusters, gamePin)
+ val clamp = PowerManager.latestTunerCaps()?.primeKhz
</file context>
| roles: Roles, | ||
| plan: PlacementPlan, | ||
| ): Map<Int, Placement> { | ||
| val byTid = threads.associateBy { it.tid } |
Contributor
There was a problem hiding this comment.
P2: Affinity state is keyed only by TID, but this scanner tracks both pid and tid; after a task exits, Linux may reuse that TID for a different process during the session. desiredPlacements then skips taking the new task's original mask, and releaseThread can restore the old task's mask to the new task; key state by (pid, tid) or invalidate it when the PID changes.
(Based on your team's feedback about preserving lifecycle identity.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At app/src/main/java/app/gamenative/powercontrol/FrameThreadPlacement.kt, line 203:
<comment>Affinity state is keyed only by TID, but this scanner tracks both `pid` and `tid`; after a task exits, Linux may reuse that TID for a different process during the session. `desiredPlacements` then skips taking the new task's original mask, and `releaseThread` can restore the old task's mask to the new task; key state by `(pid, tid)` or invalidate it when the PID changes.
(Based on your team's feedback about preserving lifecycle identity.) </comment>
<file context>
@@ -0,0 +1,551 @@
+ roles: Roles,
+ plan: PlacementPlan,
+ ): Map<Int, Placement> {
+ val byTid = threads.associateBy { it.tid }
+ val result = LinkedHashMap<Int, Placement>()
+ val frame = roles.frame?.let { byTid[it.tid] }
</file context>
Contributor
There was a problem hiding this comment.
2 existing issues remain and no new issues found across 5 files
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
Owner
Author
|
Closing: the fix should live in wine (no PServer dependency), not the app. |
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
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.
Description
On big.LITTLE devices the thread that sets the frame often does not land on the fastest core. On the Retroid Pocket 6 (Cult of the Lamb), once the spinner was throttled in wine (proton-wine PR 119), the prime core (core 7) sat at 8 % while the 72 %-busy main thread stayed on the gold cores, and fps stayed at 26.
This adds
FrameThreadPlacement. Every 2 s it reads the game's threads, wineserver and the Steam processes (steam.exe, steamservice.exe, steamhost*.exe) from /proc, sorts the threads into roles and sets their affinity withsched_setaffinity(same uid, no privilege). Nothing is configured per game.IPC:CSteamEnginethread (its comm readsIPC:CSteamEngin), and any wineserver or Steam thread above 10 % CPU. They go on every core outside the low-capacity clusters.Cpus_allowed_listit had before it was first placed. Everything placed is released at session end, and also when power control is switched off mid-session.Gate: this needs a PServer driver with cluster capacities (
placementClusters()) and the profile's power control on. Without them nothing is placed. Kill switches, with no UI: PrefManagerframeThreadPlacement(default true) and the container env varGN_FRAME_PLACEMENT=0.game_exited gets
frame_thread_tid,frame_thread_comm,spinner_tidsandframe_placement_appliedfrom the same extension point as the process telemetry. This is not gated on analytics.Unit tests cover role classification, frame stickiness, the cluster masks for the RP6, the Odin 3 and edge layouts, game-pin intersection, the window apply/release flow and the session summary.
Device verification is pending: RP6 with Cult of the Lamb (frame thread on core 7), and Odin 3 with DOOM 2016.
Recording
Not applicable (no UI change). Device verification pending.
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.Summary by cubic
Places the frame thread on the highest-capacity cluster on big.LITTLE devices so it no longer lands on a slower core and caps FPS.
FrameThreadPlacementre-reads the game's threads, wineserver, and Steam from /proc every 2 s, classifies them by role, and pins each role to a core set withsched_setaffinity(same uid, no privileges, nothing configured per game).Behavior
frameThreadPlacementpref (default true) and container env varGN_FRAME_PLACEMENT=0.frame_thread_tid,frame_thread_comm,spinner_tids, andframe_placement_applied.Written for commit 20bb456. Summary will update on new commits.
Summary by CodeRabbit