Feature/sd card download cap – Second attempt after refactoring#598
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a persisted Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as "Settings UI"
participant Pref as "PrefManager"
participant Steam as "SteamService"
User->>SettingsUI: Toggle "SD card cap"
SettingsUI->>Pref: set sdCardCap(value)
Pref-->>SettingsUI: persist (logs via Timber.d)
Steam->>Pref: read sdCardCap & useExternalStorage during budget calc
Pref-->>Steam: sdCardCap / useExternalStorage values
Steam->>Steam: compute maxDecompress (2 if sdCardCap && externalStorage, else CPU-based)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/service/SteamService.kt`:
- Around line 1492-1494: The SD-card branch can produce maxDownloads == 0 when
PrefManager.downloadSpeed < 8; update the calculation for maxDownloads (the
expression that assigns to maxDownloads where PrefManager.sdCardCap is true) to
enforce a lower bound (e.g., coerceAtLeast(1) or Math.max(1,
PrefManager.downloadSpeed / 8)) so it never becomes zero and stalls the
downloader; keep the existing non-SD-card logic using cpuCores and downloadRatio
unchanged.
| var sdCardCap by rememberSaveable {mutableStateOf(PrefManager.sdCardCap) } | ||
| SettingsSwitch( | ||
| colors = settingsTileColorsAlt(), | ||
| title = { Text(text = stringResource(R.string.settings_interface_sd_card_cap_title)) }, | ||
| subtitle = { Text(text = stringResource(R.string.settings_interface_sd_card_cap_subtitle)) }, | ||
| state = sdCardCap, | ||
| onCheckedChange = { | ||
| sdCardCap = it | ||
| PrefManager.sdCardCap = it | ||
| } | ||
|
|
||
| ) |
There was a problem hiding this comment.
Let's not do this. Instead we should check if external storage is enabled and force the cap with a reasonable value
There was a problem hiding this comment.
Thanks for the review! How about it we do it in reverse instead? The setting is automatically on for external storage and capped (so no fiddling with download threads), but it's possible to turn off through a setting only visible if using external storage? Either that or I actually try to ensure the user is trying to download to an SD card, it might be possible.
Right now it is possible someone is running an NVME SSD as external storage which would be needlessly throttled by this cap. This should only benefit SD cards and cheaper types of USB storage.
Anyway I will have do some further testing just to try to see if the optimal setting for my device is good as a generalisable one for for most users.
There was a problem hiding this comment.
I'm worried that people will try and uncap it in settings if they see it, to try and get faster downloads because they don't understand why the cap exists. Few people are likely to be using a hard disk as external storage for an Android tbh, so I think we cap by default and then expose as a value if people complain it's too slow.
Lemme know your findings
There was a problem hiding this comment.
You do have a good point. I'm pushing a new version today once I've untangled a bit of weirdness I've caused in my branch which should streamline the feature quite a bit (making it opt-out for external storage at least, and hiding the option if not using external storage). I also believe by not explicitly referring to it as a cap, but rather as an optimization might dissuade most users from turning it off.
Though I think a solution to most of my concerns would be to tune the cap to a sweet spot where I am able to say for certain an SD card will not benefit from more threads (rather than just trying to find the optimum if that makes sense). And you're right that almost no users will actually be using an external SSD, and if they do a minor loss in download performance isn't the biggest problem in the world.
With that said I do still have some loose threads to uncover before I feel I'm done with this:
- My testing has so far been pretty inconsistent and I'd like to figure out why and what it says about the cap idea
My first round of testing with a slightly faster memory card showed far lower thread counts as optimal, in later tests with a slower card I had to increase the number of download threads quite a bit (but still cap decompress) to beat uncapped. I think there might not be a good one size fits all solution for download threads. And I'm not sure you actually gain download performance in every single case, although caps always improves stability while downloading.
- I need to look more into how users are actually experiencing the issues I'm describing
For me uncapped downloads to SD cards have been an awful experience in terms of performance and stability, which is the reason I started this project. But right now I feel like I need to try to document if these issues are actually universal. Someone in the discord mentioned they did not experience stability issues, only performance issues. Which is strange to me, and I need to figure out why.
- I am no longer certain about my initial assumptions about the issues
Initially I thought that the issue was a simple I/O jam of to many threads trying to do too many write operations on the SD card at the same time, but reading more about the DepotDownloader I saw writes are already capped to 1 by default, and I see no reason to believe GN changes that. This is per instance of DepotDownloader and I am still not sure if it's possible to have more than one instance at a time. However if FileWrites are truly capped at one that could change the assumption from a simply I/O jam to a more nuanced issue. One hypothesis I have is that given only one write thread and a swarm of download and decompression threads it could be that 1. They're all fighting for a chance to write their chunk 2. It could be that so much of it gets clogged in the RAM that the garbage collector starts going crazy (which has some support from the logs) leading to random chunks constantly being purged before they're written
I apologise for being so long winded in my writing. I'm new to this and want to make sure I capture what's relevant, coupled with my propensity for thinking out loud, but the tl;dr is I'm going to investigate a little bit longer to make sure I actually both understand the issue and solution better.
There was a problem hiding this comment.
Oh never mind actually, now I get it now or at least more of it (I think). I didn't catch that since #425 we're no longer using the main fork of JavaSteam. The reason why my initial testing looked very different, is because I had previously built my own version of JavaSteam based on the latest mainline version. This DepotDownloader works differently though, and doesn't have a maxFileWrites, which to my understanding necessitates a thread cap even more however to achieve a similar effect.
I'll have to study this further.
There was a problem hiding this comment.
Oh I see, so does the cap not work on main JS?
There was a problem hiding this comment.
My cap works on either, but it could explain why main JS and GN's fork of JS have different optimal thread caps. Even if fileWrites are constrained on main, the main is way less memory efficient which should only be exacerbated by high download/decompress threads
There was a problem hiding this comment.
So update: I've my latest push is just the original fix I set out to make, agnostic towards what happens upstreams with JavaSteam. However I have created an extra branch containing the changes that do actually become specific to my updated version of JavaSteam that I need to work on implementing with JT first, then merge with GameNative (if that makes sense)
There was a problem hiding this comment.
Also I've made my git branches needlessly convoluted so I need to make sure I've actually pushed what I intended to
| val maxDownloads = if (PrefManager.sdCardCap) (PrefManager.downloadSpeed/8).coerceAtLeast(1) else (cpuCores * downloadRatio).toInt().coerceAtLeast(1) | ||
| val maxDecompress = if (PrefManager.sdCardCap) 1 else (cpuCores * decompressRatio).toInt().coerceAtLeast(1) |
There was a problem hiding this comment.
check if external storage enabled, force cap with a reasonable number if so.
…etely additive and far simpler
…storage is not selected, and decided on optimal config after testing
ProRapSuperstarOriginalMaster
force-pushed
the
feature/sd-card-download-cap
branch
from
b2316c0 to
ace525f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/PrefManager.kt (1)
829-835: Remove theTimber.dlog from the setter, or apply it consistently across all setters.No other property setter in
PrefManagerincludes a log statement —sdCardCapis the only exception. This adds noise inconsistently and could surprise future readers who expect uniform setter behaviour across the file.♻️ Proposed cleanup
private val SD_CARD_CAP = booleanPreferencesKey("sd_card_cap") var sdCardCap: Boolean get() = getPref(SD_CARD_CAP, false) set(value) { - Timber.d("Setting SD_CARD_CAP to $value") setPref(SD_CARD_CAP, value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/PrefManager.kt` around lines 829 - 835, In PrefManager remove the inconsistent debug log from the sdCardCap setter (the Timber.d("Setting SD_CARD_CAP to $value") call) so its behavior matches other property setters; locate the SD_CARD_CAP key and the sdCardCap var and delete the Timber.d line (or alternatively, if you prefer logs, add equivalent Timber.d calls to all other setters for consistency, but default to removing this lone log).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/app/gamenative/PrefManager.kt`:
- Around line 829-835: In PrefManager remove the inconsistent debug log from the
sdCardCap setter (the Timber.d("Setting SD_CARD_CAP to $value") call) so its
behavior matches other property setters; locate the SD_CARD_CAP key and the
sdCardCap var and delete the Timber.d line (or alternatively, if you prefer
logs, add equivalent Timber.d calls to all other setters for consistency, but
default to removing this lone log).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/PrefManager.ktapp/src/main/java/app/gamenative/service/SteamService.ktapp/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/res/values/strings.xml
- app/src/main/java/app/gamenative/service/SteamService.kt
- app/src/main/java/app/gamenative/ui/screen/settings/SettingsGroupInterface.kt
|
Let me know how we want to proceed with this one. |
2 participants
I've attempted to recreate the conditions that lead the tests to fail when attempting my previous pull requests, but I can't seem to get it to fail on my machine. To remove some possible sources of error I have hard reset my cloned repo and added my logic back. Then I've also refactored it to make the code as additive as I am able to and not to disrupt any other logic. That way hopefully my test can now succeed. Apologies in advance for my poor Git skills, I do not wish to make code reviews any harder for whomever may read this. If I am wrong to try to commit this, please tell me to stop.
--- Description from previous PR ---
"I added a setting to cap the decompression thread to 1, as well as significantly lowering the effect of the download speed setting on download threads. The purpose of this setting is to adapt these to the limitations of simpler storage mediums (primarily micro SD cards), and avoid I/O jams that negatively affects download performance and app stability.
Through my own testing this solves an issue where the app interface would become extremely unstable and crash frequently during downloads. The gains in stability and lowering download performance to the hardware bottleneck seem to counteract any losses to speed. And through my own testing as well as some rudimentary research into the workings of exFAT formatted SD cards I have determined that decompression threads should not exceed 1, and that download threads benefit from being higher than the decompression threads. Likely due to decompression being faster than downloads, and allowing the decompression thread a buffer to always work at full capacity.
I have decided to keep the download threads variable through the speed setting, as I believe the optimal download threads will also depend on network speeds. I have also decided not to lock the setting behind the condition of external storage being used, not force it on for external storage. As I believe there could be internal storage mediums that also benefit from this setting, and external storage mediums that don't.
P.S. This is my first ever commit/pull request to an open source repo, apologies in advance if I've done anything wrong or if my code is bad."
Summary by cubic
Adds an SD card optimization that caps decompression threads to 2 when writing to external storage to improve stability on slower media. The cap auto-enables when external storage is selected and can be toggled in Settings.
New Features
Bug Fixes
Written for commit 813bd89. Summary will update on new commits.
Summary by CodeRabbit
New Features
Persistence
Documentation