Fix compatibility bugs and performance issues#668
Closed
gigachadmma69 wants to merge 2 commits intoutkarshdalal:masterfrom
Closed
Fix compatibility bugs and performance issues#668gigachadmma69 wants to merge 2 commits intoutkarshdalal:masterfrom
gigachadmma69 wants to merge 2 commits intoutkarshdalal:masterfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces unbounded/thread-unsafe caches with bounded or concurrent variants, reduces GameCompatibilityCache TTL to 6 hours, scopes HTTP response handling with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/utils/BestConfigService.kt`:
- Around line 132-136: The eviction code using cache (a ConcurrentHashMap) with
MAX_CACHE_SIZE doesn't evict the oldest entries because ConcurrentHashMap is
unordered and the size/keys() check has a race; replace the
ConcurrentHashMap-based manual eviction with a synchronized LinkedHashMap-based
bounded cache by initializing cache as Collections.synchronizedMap(object :
LinkedHashMap<String, BestConfigResponse>(MAX_CACHE_SIZE, 0.75f, false) {
override fun removeEldestEntry(eldest) = size > MAX_CACHE_SIZE }) and remove the
manual eviction block that computes keysToRemove, or if you prefer to keep
ConcurrentHashMap, update the comment to state that eviction is arbitrary and
(optionally) track insertion timestamps to implement true FIFO using a
PriorityQueue or LinkedHashMap-backed structure instead.
In `@app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt`:
- Around line 18-20: The plain Boolean cacheLoaded is not thread-safe and can
race in loadCache(); replace it with a thread-safe flag (e.g., an AtomicBoolean
or mark it `@Volatile`) and update loadCache() to atomically check-and-set the
flag (use AtomicBoolean.compareAndSet or synchronized block) so only one
coroutine proceeds to populate inMemoryCache and timestamps; ensure any
early-return logic uses the same atomic check and clear any existing non-atomic
reads of cacheLoaded.
In `@app/src/main/java/com/winlator/core/GPUInformation.java`:
- Around line 33-43: getGpuCards currently calls FileUtils.readString(context,
"gpu_cards.json") and passes the result directly to new JSONArray, but
FileUtils.readString can return null on IO errors which will cause a
NullPointerException not caught by the JSONException handler; update getGpuCards
(and use cachedGpuCards) to check if gpuNameList is null or empty before
constructing the JSONArray and fall back to new JSONArray() in that case (or
when parsing fails), and ensure the try/catch covers both JSONException and the
null case so cachedGpuCards is always initialized.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/utils/BestConfigService.ktapp/src/main/java/app/gamenative/utils/GameCompatibilityCache.ktapp/src/main/java/app/gamenative/utils/GameCompatibilityService.ktapp/src/main/java/com/winlator/core/GPUInformation.javaapp/src/main/java/com/winlator/xserver/Drawable.java
💤 Files with no reviewable changes (1)
- app/src/main/java/com/winlator/xserver/Drawable.java
Comment on lines
18
to
20
| private val inMemoryCache = ConcurrentHashMap<String, GameCompatibilityService.GameCompatibilityResponse>() | ||
| private val timestamps = ConcurrentHashMap<String, Long>() | ||
| private var cacheLoaded = false |
Contributor
There was a problem hiding this comment.
Thread-safety gap with cacheLoaded flag.
While inMemoryCache and timestamps are now thread-safe ConcurrentHashMap instances, the cacheLoaded flag on line 20 is a plain Boolean without synchronization. Multiple coroutines could race past the if (cacheLoaded) return check in loadCache(), causing duplicate initialization.
Consider using @Volatile or an AtomicBoolean, or synchronizing the entire loadCache() method:
Proposed fix using AtomicBoolean
+import java.util.concurrent.atomic.AtomicBoolean
import java.util.concurrent.ConcurrentHashMap
...
- private var cacheLoaded = false
+ private val cacheLoaded = AtomicBoolean(false)
...
private fun loadCache() {
- if (cacheLoaded) return
+ if (!cacheLoaded.compareAndSet(false, true)) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt` around
lines 18 - 20, The plain Boolean cacheLoaded is not thread-safe and can race in
loadCache(); replace it with a thread-safe flag (e.g., an AtomicBoolean or mark
it `@Volatile`) and update loadCache() to atomically check-and-set the flag (use
AtomicBoolean.compareAndSet or synchronized block) so only one coroutine
proceeds to populate inMemoryCache and timestamps; ensure any early-return logic
uses the same atomic check and clear any existing non-atomic reads of
cacheLoaded.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files
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/utils/BestConfigService.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/BestConfigService.kt:132">
P3: The comment says "Evict oldest entries" but `ConcurrentHashMap` does not maintain insertion order. `cache.keys().toList().take(...)` will remove arbitrary entries, not the oldest. Either update the comment to reflect the actual behavior (arbitrary eviction) or use a `LinkedHashMap` with synchronization for true FIFO eviction.</violation>
</file>
<file name="app/src/main/java/com/winlator/core/GPUInformation.java">
<violation number="1" location="app/src/main/java/com/winlator/core/GPUInformation.java:36">
P1: Missing null check for file read result may lead to NullPointerException. FileUtils.readString() returns null when the file doesn't exist or can't be read. If gpuNameList is null, new JSONArray(null) will throw an unchecked NullPointerException that won't be caught by the JSONException handler, causing the app to crash.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt:18">
P2: Thread-safety fix is incomplete: while the maps are now `ConcurrentHashMap`, the `cacheLoaded` flag remains a plain `Boolean` without any synchronization. Multiple coroutines could race past the `if (cacheLoaded) return` check in `loadCache()`, causing duplicate initialization. Consider using `AtomicBoolean` with `compareAndSet` or marking it `@Volatile` at minimum.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt (1)
20-20:⚠️ Potential issue | 🟠 Major
@Volatilealone still leaves a racy cache initialization path.At Line 71, the
if (cacheLoaded) returncheck is non-atomic with the subsequent population/writes. Concurrent callers can initialize in parallel, and one initializer can overwrite fresher entries written by another thread duringcache()/cacheAll().Suggested fix (single-flight initialization via lock)
object GameCompatibilityCache { @@ private val inMemoryCache = ConcurrentHashMap<String, GameCompatibilityService.GameCompatibilityResponse>() private val timestamps = ConcurrentHashMap<String, Long>() `@Volatile` private var cacheLoaded = false + private val loadLock = Any() @@ private fun loadCache() { if (cacheLoaded) return - - try { - val cacheJson = PrefManager.gameCompatibilityCache - if (cacheJson.isEmpty() || cacheJson == "{}") { - cacheLoaded = true - return - } - - val cacheMap = Json.decodeFromString<Map<String, CachedCompatibilityResponse>>(cacheJson) - - // Load all entries into memory (no expiration check here - lazy expiration) - // Store both response and timestamp for expiration checking - cacheMap.forEach { (gameName, cached) -> - inMemoryCache[gameName] = cached.response.toResponse() - timestamps[gameName] = cached.timestamp - } - - Timber.tag("GameCompatibilityCache").d("Loaded ${inMemoryCache.size} cached entries from persistent storage") - cacheLoaded = true - } catch (e: Exception) { - Timber.tag("GameCompatibilityCache").e(e, "Failed to load cache from persistent storage") - cacheLoaded = true // Mark as loaded to avoid retrying + synchronized(loadLock) { + if (cacheLoaded) return + try { + val cacheJson = PrefManager.gameCompatibilityCache + if (cacheJson.isEmpty() || cacheJson == "{}") { + cacheLoaded = true + return + } + + val cacheMap = Json.decodeFromString<Map<String, CachedCompatibilityResponse>>(cacheJson) + cacheMap.forEach { (gameName, cached) -> + inMemoryCache[gameName] = cached.response.toResponse() + timestamps[gameName] = cached.timestamp + } + Timber.tag("GameCompatibilityCache").d("Loaded ${inMemoryCache.size} cached entries from persistent storage") + } catch (e: Exception) { + Timber.tag("GameCompatibilityCache").e(e, "Failed to load cache from persistent storage") + } finally { + cacheLoaded = true + } } }Run this to verify the current non-atomic check/set path is still present:
#!/bin/bash # Expected: you'll see `if (cacheLoaded) return` and separate `cacheLoaded = true` # without synchronization around the whole initialization block. FILE="app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt" sed -n '65,110p' "$FILE" echo "----" rg -nP 'if\s*\(cacheLoaded\)\s*return|cacheLoaded\s*=\s*true|@Synchronized\s+private\s+fun\s+loadCache|synchronized\s*\(' "$FILE"Also applies to: 70-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt` at line 20, The cacheLoaded volatile flag is checked and set non-atomically causing racey initialization in GameCompatibilityCache; wrap the entire initialization path in a single-flight lock so the "if (cacheLoaded) return" check and the population (calls to cache() / cacheAll()) plus the assignment "cacheLoaded = true" occur under the same synchronization. Concretely, add a private lock object or make loadCache synchronized (e.g., synchronize around a private Any() or use `@Synchronized` on loadCache), move the early-return check inside that synchronized block, perform cache/cacheAll() there, then set cacheLoaded = true before leaving the block to ensure only one thread initializes and others return immediately. Ensure callers still call the same entrypoint (loadCache or init method) so no other code paths bypass the lock.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/BestConfigService.kt (1)
125-125: Add a warning for null response bodies.Line 125 returns
nullsilently when the body is absent; adding a warning here would improve production diagnostics.🔧 Suggested tweak
- val responseBody = it.body?.string() ?: return@withTimeout null + val responseBody = it.body?.string() + if (responseBody == null) { + Timber.tag("BestConfigService").w("API response body is null") + return@withTimeout null + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/BestConfigService.kt` at line 125, The silent early return on null response body in BestConfigService (the line assigning responseBody: val responseBody = it.body?.string() ?: return@withTimeout null) should emit a warning before returning; update the withTimeout block to check for it.body == null and call the module's logging facility (e.g., logger.warn / Timber.w / processLogger.warn used elsewhere in this file) with a descriptive message including context (endpoint/request id if available) and then return@withTimeout null so production diagnostics capture when responses have no body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt`:
- Line 20: The cacheLoaded volatile flag is checked and set non-atomically
causing racey initialization in GameCompatibilityCache; wrap the entire
initialization path in a single-flight lock so the "if (cacheLoaded) return"
check and the population (calls to cache() / cacheAll()) plus the assignment
"cacheLoaded = true" occur under the same synchronization. Concretely, add a
private lock object or make loadCache synchronized (e.g., synchronize around a
private Any() or use `@Synchronized` on loadCache), move the early-return check
inside that synchronized block, perform cache/cacheAll() there, then set
cacheLoaded = true before leaving the block to ensure only one thread
initializes and others return immediately. Ensure callers still call the same
entrypoint (loadCache or init method) so no other code paths bypass the lock.
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/utils/BestConfigService.kt`:
- Line 125: The silent early return on null response body in BestConfigService
(the line assigning responseBody: val responseBody = it.body?.string() ?:
return@withTimeout null) should emit a warning before returning; update the
withTimeout block to check for it.body == null and call the module's logging
facility (e.g., logger.warn / Timber.w / processLogger.warn used elsewhere in
this file) with a descriptive message including context (endpoint/request id if
available) and then return@withTimeout null so production diagnostics capture
when responses have no body.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/utils/BestConfigService.ktapp/src/main/java/app/gamenative/utils/GameCompatibilityCache.ktapp/src/main/java/com/winlator/core/GPUInformation.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/core/GPUInformation.java
- Remove unreachable dead code after return statements in
BestConfigService.validateComponentVersions() (7 instances of
filteredJson.put() after return that never executed)
- Fix OkHttp response body leaks in BestConfigService and
GameCompatibilityService by wrapping responses in use{} blocks
- Fix double forceUpdate() call in Drawable.drawImage() for depth
24/32 that caused unnecessary texture re-uploads during gameplay
- Fix thread safety in GameCompatibilityCache by using
ConcurrentHashMap instead of mutableMapOf()
- Cache parsed gpu_cards.json in GPUInformation to avoid expensive
re-parsing on every getDeviceIdFromGPUName/getVendorIdFromGPUName call
- Add MAX_CACHE_SIZE (100) eviction to BestConfigService to prevent
unbounded memory growth
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- BestConfigService: replace ConcurrentHashMap with synchronized LinkedHashMap for proper FIFO cache eviction - GameCompatibilityCache: mark cacheLoaded as @volatile - GPUInformation: null-check FileUtils.readString before JSONArray ctor
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
2 participants
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.
Summary
BestConfigService.validateComponentVersions()— 7 instances offilteredJson.put()afterreturnstatements that never executed. These were intended fallbacks that silently failed, meaning games with missing component versions would get no fallback config applied.BestConfigService.fetchBestConfig()andGameCompatibilityService.fetchCompatibility()by wrapping responses inuse{}blocks. Without this, HTTP connections leak and eventually exhaust the connection pool.forceUpdate()call inDrawable.drawImage()for depth 24/32. The method calledforceUpdate()inside the depth branch AND unconditionally after it, causing unnecessary duplicate texture re-uploads during gameplay (performance hit on every frame that draws images).GameCompatibilityCacheby replacingmutableMapOf()withConcurrentHashMap. The original maps could throwConcurrentModificationExceptionwhen accessed from multiple coroutines.gpu_cards.jsoninGPUInformationvia a synchronized lazy singleton. Previously, every call togetDeviceIdFromGPUName()orgetVendorIdFromGPUName()re-read and re-parsed the JSON from assets.BestConfigServicewith aMAX_CACHE_SIZEof 100 entries to prevent unbounded memory growth in long sessions.Files Changed
BestConfigService.ktresponse.use{}, add cache evictionGameCompatibilityService.ktresponse.use{}GameCompatibilityCache.ktmutableMapOf→ConcurrentHashMapGPUInformation.javagetGpuCards()methodDrawable.javaforceUpdate()callTest plan
🤖 Generated with Claude Code
Summary by cubic
Fixes HTTP leaks and thread-safety issues, ensures fallback configs apply, and improves rendering and caching. Adds FIFO cache eviction and a 6-hour TTL to reduce memory use and stale compatibility data.
Bug Fixes
Performance
Written for commit f813af1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Performance Improvements
Rendering