Reduce Epic downloader CPU and thermal overhead on tablets#1599
Reduce Epic downloader CPU and thermal overhead on tablets#1599Misazam wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesEpicDownloadManager Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 882-883: The parallelDownloads and parallelAssemble variables in
the EpicDownloadManager.kt file only use coerceAtMost for upper bounds but are
missing lower bounds. If speedConfig.maxDownloads or speedConfig.maxDecompress
returns 0, this would result in flatMapMerge receiving concurrency of 0 and
boundedFlowCapacity being 0, which prevents any items from being processed. Add
coerceAtLeast(1) to both the parallelDownloads and parallelAssemble assignments
to match the pattern already used in downloadOverlay, ensuring these concurrency
values never drop below 1.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f37f67a9-d5b5-4015-8d02-5cdf00bc7908
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt
![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
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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 `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 916-918: The MutableSharedFlow declarations for networkChunkFlow
and assembleFlow at lines 916-918 use extraBufferCapacity set to Int.MAX_VALUE,
which creates unbounded queues that bypass proper backpressure. Replace the
Int.MAX_VALUE values with a bounded capacity derived from the downloader's
concurrency levels to restore backpressure safety. Additionally, locate all
tryEmit() calls associated with these flows (including at lines 969, 997, 1020,
and 1103) and replace them with emit() to ensure proper backpressure handling in
the suspend context, preventing silent drops of work and deadlocks in
pendingChunks tracking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4101a7e5-8f78-46aa-ac2f-fc101b31f582
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt`:
- Around line 916-918: The MutableSharedFlow declarations for networkChunkFlow
and assembleFlow at lines 916-918 use extraBufferCapacity set to Int.MAX_VALUE,
which creates unbounded queues that bypass proper backpressure. Replace the
Int.MAX_VALUE values with a bounded capacity derived from the downloader's
concurrency levels to restore backpressure safety. Additionally, locate all
tryEmit() calls associated with these flows (including at lines 969, 997, 1020,
and 1103) and replace them with emit() to ensure proper backpressure handling in
the suspend context, preventing silent drops of work and deadlocks in
pendingChunks tracking.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4101a7e5-8f78-46aa-ac2f-fc101b31f582
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt
🛑 Comments failed to post (1)
app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt (1)
916-918:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Read-only verification for Epic downloader SharedFlow buffering/emission. # Expect: no Int.MAX_VALUE buffer capacity in EpicDownloadManager queues, and retry/result paths use emit(...) or handle tryEmit(...) == false. rg -n -C3 'MutableSharedFlow<|extraBufferCapacity\s*=\s*Int\.MAX_VALUE|\.tryEmit\s*\(' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktRepository: utkarshdalal/GameNative
Length of output: 2144
🏁 Script executed:
#!/bin/bash # Check if tryEmit() return values are being handled/checked in the identified lines echo "=== Checking tryEmit() handling at lines 969, 997, 1020, 1103 ===" rg -n -B2 -A2 '(assembleFlow|networkChunkFlow)\.tryEmit' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt echo "" echo "=== Searching for parallelDownloads and parallelAssemble variable definitions ===" rg -n 'val\s+(parallelDownloads|parallelAssemble)' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt echo "" echo "=== Check context around line 916 for variable scope ===" sed -n '880,920p' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktRepository: utkarshdalal/GameNative
Length of output: 3720
🏁 Script executed:
#!/bin/bash # Understand the flow context and verify if emit() suspension is safe echo "=== Check flow declaration context and collectors ===" sed -n '916,960p' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt echo "" echo "=== Check how networkChunkFlow and assembleFlow are collected/processed ===" rg -n -A3 'networkChunkFlow\.|assembleFlow\.' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt | head -60 echo "" echo "=== Check pendingChunks decrement logic ===" rg -n 'pendingChunks' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.ktRepository: utkarshdalal/GameNative
Length of output: 3565
🏁 Script executed:
#!/bin/bash # Check context around line 1065 where emit() is used (not tryEmit) echo "=== Context around line 1065 where emit() is used ===" sed -n '1055,1075p' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt echo "" echo "=== Look for assembleFlow collection/consumption ===" rg -n -B5 -A10 'assembleFlow\s*\.' app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt | grep -A15 '\.collect\|\.onEach\|\.forEach'Repository: utkarshdalal/GameNative
Length of output: 1256
Restore bounded backpressure for the downloader queues.
extraBufferCapacity = Int.MAX_VALUEmakes the network/result queues effectively unbounded, allowing pre-allocation and retry re-emission to enqueue large manifests without backpressure. The ignoredtryEmit(...)calls are also unsafe once the buffer is bounded, as a failed emit would silently drop work and leavependingChunksstuck.Replace the unbounded capacity with a bounded value derived from concurrency levels, and switch all
tryEmit()calls toemit()to ensure backpressure safety in the suspend context.Proposed fix
+ val boundedFlowCapacity = ((parallelDownloads + parallelAssemble) * 4).coerceAtLeast(1) val networkChunkFlow = MutableSharedFlow<app.gamenative.service.epic.manifest.ChunkInfo>(extraBufferCapacity = Int.MAX_VALUE) val assembleFlow = MutableSharedFlow<Pair<app.gamenative.service.epic.manifest.ChunkInfo, Result<File>>>(extraBufferCapacity = Int.MAX_VALUE)- val networkChunkFlow = MutableSharedFlow<app.gamenative.service.epic.manifest.ChunkInfo>(extraBufferCapacity = Int.MAX_VALUE) + val networkChunkFlow = MutableSharedFlow<app.gamenative.service.epic.manifest.ChunkInfo>( + extraBufferCapacity = boundedFlowCapacity, + ) val assembleFlow = - MutableSharedFlow<Pair<app.gamenative.service.epic.manifest.ChunkInfo, Result<File>>>(extraBufferCapacity = Int.MAX_VALUE) + MutableSharedFlow<Pair<app.gamenative.service.epic.manifest.ChunkInfo, Result<File>>>( + extraBufferCapacity = boundedFlowCapacity, + )- assembleFlow.tryEmit(chunk to result) + assembleFlow.emit(chunk to result)- networkChunkFlow.tryEmit(chunk) + networkChunkFlow.emit(chunk)- networkChunkFlow.tryEmit(chunk) + networkChunkFlow.emit(chunk)- missingChunks.forEach { networkChunkFlow.tryEmit(it) } + missingChunks.forEach { networkChunkFlow.emit(it) }Also applies to lines 969, 997, 1020, 1103.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.val boundedFlowCapacity = ((parallelDownloads + parallelAssemble) * 4).coerceAtLeast(1) val networkChunkFlow = MutableSharedFlow<app.gamenative.service.epic.manifest.ChunkInfo>( extraBufferCapacity = boundedFlowCapacity, ) val assembleFlow = MutableSharedFlow<Pair<app.gamenative.service.epic.manifest.ChunkInfo, Result<File>>>( extraBufferCapacity = boundedFlowCapacity, )🤖 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 `@app/src/main/java/app/gamenative/service/epic/EpicDownloadManager.kt` around lines 916 - 918, The MutableSharedFlow declarations for networkChunkFlow and assembleFlow at lines 916-918 use extraBufferCapacity set to Int.MAX_VALUE, which creates unbounded queues that bypass proper backpressure. Replace the Int.MAX_VALUE values with a bounded capacity derived from the downloader's concurrency levels to restore backpressure safety. Additionally, locate all tryEmit() calls associated with these flows (including at lines 969, 997, 1020, and 1103) and replace them with emit() to ensure proper backpressure handling in the suspend context, preventing silent drops of work and deadlocks in pendingChunks tracking.
|
@joshuatam thoughts here? I think we should be using the settings for the download speed for the parallelism, not hardcoding it like this @Misazam |
| val parallelDownloads = PrefManager.downloadSpeed | ||
| .coerceAtLeast(1) | ||
| .coerceAtMost(EPIC_MAX_PARALLEL_DOWNLOADS) |
| private const val EPIC_MAX_PARALLEL_DOWNLOADS = 6 | ||
| private const val EPIC_MAX_PARALLEL_ASSEMBLY = 2 |
There was a problem hiding this comment.
Hardcoding with max values would only help reducing CPU usage, but would also slow down the download throughput. did you compare real game download time in this PR?
There was a problem hiding this comment.
Besides, if you tried set the Download Speed in app setting to Medium, the device will be cooler similar to your test result.
|
Thanks for the feedback. I understand the concern about Epic's specific speed limit, and I'm not attached to that particular implementation. What I need is guidance on the appropriate official solution for this issue. My device is a OnePlus Pad 3, a high-end Android tablet. During large Epic installations, the current official behavior can cause the tablet to overheat for extended periods. I'm concerned not only about download speed but also battery drain and long-term hardware stress during very large downloads. If this occurs on a high-end tablet, I'm worried that the Epic download manager's behavior will be even worse on more modest devices. If the proposed solution is for "users to manually set the download speed to medium for large Epic downloads," I can accept that, but I think the app should indicate this more clearly or, possibly, handle fragmented Epic installations more conservatively by default. Epic downloads aren't just network downloads; They also involve fragment decompression, file verification, and assembly, so the CPU/thermal cost is different from a simple download. I don't want to maintain a local fork indefinitely just to protect my tablet with every GameNative update. If this pull request isn't suitable, could you suggest a design you consider acceptable? For example:
My main goal is simple: I want to be able to use official GameNative versions on my tablet without having to reapply the Epic downloader's local optimizations after every update. |
|
@Misazam - you should see the settings of the app. There's a slider there for download speed which says that it trades off between download speed and heat. This setting is used in Steam downloads for the parallelism, like you are doing here. The point is that it should be configurable by the user. |
|
I updated the PR to follow the existing Download Speed setting instead of hardcoding Epic-specific caps. Epic now uses Validated locally with |
3 participants
Description
This is a follow-up to #1036, rebased/adapted after the recent Epic downloader refactor in #1554.
The current Flow-based Epic downloader is faster and more reliable than the previous implementation, but on large chunked installs it can still create excessive local CPU/thermal overhead on thermally constrained Android tablets.
Test device:
What I observed on current master during a real Epic download:
app.gamenativearound ~180–225% CPUDefaultDispatcher/HeapTaskDaemonactivityThis patch reduces local downloader overhead while preserving the core download behavior.
It does not change:
What changed
DownloadSpeedConfigfor Epic parallelism instead of hardcoded Epic-specific caps.Recording
I do not have a full screen recording yet, but I can attach terminal/logcat screenshots from the on-device test if needed.
Type of Change
Validation
Built locally:
:app:compileModernDebugKotlinInstalled and tested on-device with a real Epic NBA 2K21 download before the last review adjustment.
Patched behavior observed during that on-device test:
11.41%to11.75%in 30 seconds21.24%downloaded and still stableapp.gamenativearound ~96–129% CPU0Latest PR revision validation:
:app:compileModernDebugKotlinThe latest revision removes Epic-specific hardcoded parallelism caps and now follows the app's existing Download Speed setting via
DownloadSpeedConfig.The goal is not to maximize peak throughput at all costs. The goal is to keep large Epic downloads stable and usable on Android tablets without creating unnecessary sustained CPU/thermal pressure, while still respecting the user's configured Download Speed setting.
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.CONTRIBUTING.md.