Fix compatibility bugs and performance issues#668
Open
gigachadmma69 wants to merge 1 commit intoutkarshdalal:masterfrom
Open
Fix compatibility bugs and performance issues#668gigachadmma69 wants to merge 1 commit intoutkarshdalal:masterfrom
gigachadmma69 wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
- 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>
Contributor
📝 WalkthroughWalkthroughThe PR refactors memory and resource management across service layers by implementing cache eviction in BestConfigService, reducing GameCompatibilityCache TTL from 7 days to 6 hours, switching internal maps to ConcurrentHashMap for thread safety, adopting response.use() patterns for proper HTTP resource cleanup across multiple services, optimizing GPU data caching, and adjusting update timing in graphics operations. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
+132
to
+136
| // Evict oldest entries if cache is full | ||
| if (cache.size >= MAX_CACHE_SIZE) { | ||
| val keysToRemove = cache.keys().toList().take(cache.size - MAX_CACHE_SIZE + 1) | ||
| keysToRemove.forEach { cache.remove(it) } | ||
| } |
Contributor
There was a problem hiding this comment.
Cache eviction does not evict "oldest" entries.
ConcurrentHashMap does not maintain insertion order, so cache.keys().toList().take(...) removes arbitrary entries rather than the oldest ones as the comment suggests. Additionally, there's a potential race between the size check and keys().toList().
For truly FIFO eviction, consider using LinkedHashMap with synchronization, or track insertion timestamps. If arbitrary eviction is acceptable (it does bound memory growth), update the comment to reflect that behavior.
Option 1: Fix comment to reflect actual behavior
- // Evict oldest entries if cache is full
+ // Evict entries if cache is full (arbitrary order, not FIFO)
if (cache.size >= MAX_CACHE_SIZE) {Option 2: Use Collections.synchronizedMap with LinkedHashMap for FIFO
// At class level:
private val cache = Collections.synchronizedMap(
object : LinkedHashMap<String, BestConfigResponse>(MAX_CACHE_SIZE, 0.75f, false) {
override fun removeEldestEntry(eldest: MutableMap.MutableEntry<String, BestConfigResponse>): Boolean {
return size > MAX_CACHE_SIZE
}
}
)This auto-evicts the eldest entry when size exceeds the limit.
📝 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.
Suggested change
| // Evict oldest entries if cache is full | |
| if (cache.size >= MAX_CACHE_SIZE) { | |
| val keysToRemove = cache.keys().toList().take(cache.size - MAX_CACHE_SIZE + 1) | |
| keysToRemove.forEach { cache.remove(it) } | |
| } | |
| // Evict entries if cache is full (arbitrary order, not FIFO) | |
| if (cache.size >= MAX_CACHE_SIZE) { | |
| val keysToRemove = cache.keys().toList().take(cache.size - MAX_CACHE_SIZE + 1) | |
| keysToRemove.forEach { cache.remove(it) } | |
| } |
🤖 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` around lines 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.
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.
Comment on lines
+33
to
+43
| private static synchronized JSONArray getGpuCards(Context context) { | ||
| if (cachedGpuCards == null) { | ||
| try { | ||
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | ||
| cachedGpuCards = new JSONArray(gpuNameList); | ||
| } catch (JSONException e) { | ||
| cachedGpuCards = new JSONArray(); | ||
| } | ||
| } | ||
| return cachedGpuCards; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential NullPointerException if asset read fails.
FileUtils.readString() returns null on IOException (see FileUtils.java lines 39-46). Passing null to new JSONArray(gpuNameList) on line 37 will throw a NullPointerException, which is not caught by the JSONException handler.
Proposed fix with null check
private static synchronized JSONArray getGpuCards(Context context) {
if (cachedGpuCards == null) {
try {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
+ if (gpuNameList == null) {
+ cachedGpuCards = new JSONArray();
+ return cachedGpuCards;
+ }
cachedGpuCards = new JSONArray(gpuNameList);
} catch (JSONException e) {
cachedGpuCards = new JSONArray();
}
}
return cachedGpuCards;
}📝 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.
Suggested change
| private static synchronized JSONArray getGpuCards(Context context) { | |
| if (cachedGpuCards == null) { | |
| try { | |
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | |
| cachedGpuCards = new JSONArray(gpuNameList); | |
| } catch (JSONException e) { | |
| cachedGpuCards = new JSONArray(); | |
| } | |
| } | |
| return cachedGpuCards; | |
| } | |
| private static synchronized JSONArray getGpuCards(Context context) { | |
| if (cachedGpuCards == null) { | |
| try { | |
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); | |
| if (gpuNameList == null) { | |
| cachedGpuCards = new JSONArray(); | |
| return cachedGpuCards; | |
| } | |
| cachedGpuCards = new JSONArray(gpuNameList); | |
| } catch (JSONException e) { | |
| cachedGpuCards = new JSONArray(); | |
| } | |
| } | |
| return cachedGpuCards; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/winlator/core/GPUInformation.java` around lines 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.
![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.
| private static synchronized JSONArray getGpuCards(Context context) { | ||
| if (cachedGpuCards == null) { | ||
| try { | ||
| String gpuNameList = FileUtils.readString(context, "gpu_cards.json"); |
Contributor
There was a problem hiding this comment.
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.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/java/com/winlator/core/GPUInformation.java, line 36:
<comment>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.</comment>
<file context>
@@ -27,11 +27,25 @@ public abstract class GPUInformation {
+ private static synchronized JSONArray getGpuCards(Context context) {
+ if (cachedGpuCards == null) {
+ try {
+ String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
+ cachedGpuCards = new JSONArray(gpuNameList);
+ } catch (JSONException e) {
</file context>
|
|
||
| private val inMemoryCache = mutableMapOf<String, GameCompatibilityService.GameCompatibilityResponse>() | ||
| private val timestamps = mutableMapOf<String, Long>() | ||
| private val inMemoryCache = ConcurrentHashMap<String, GameCompatibilityService.GameCompatibilityResponse>() |
Contributor
There was a problem hiding this comment.
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.
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/utils/GameCompatibilityCache.kt, line 18:
<comment>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.</comment>
<file context>
@@ -6,16 +6,17 @@ import kotlinx.serialization.encodeToString
- private val inMemoryCache = mutableMapOf<String, GameCompatibilityService.GameCompatibilityResponse>()
- private val timestamps = mutableMapOf<String, Long>()
+ private val inMemoryCache = ConcurrentHashMap<String, GameCompatibilityService.GameCompatibilityResponse>()
+ private val timestamps = ConcurrentHashMap<String, Long>()
private var cacheLoaded = false
</file context>
| matchedGpu = jsonResponse.getString("matchedGpu"), | ||
| matchedDeviceId = jsonResponse.getInt("matchedDeviceId") | ||
| ) | ||
| // Evict oldest entries if cache is full |
Contributor
There was a problem hiding this comment.
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.
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/utils/BestConfigService.kt, line 132:
<comment>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.</comment>
<file context>
@@ -108,32 +109,40 @@ object BestConfigService {
- matchedGpu = jsonResponse.getString("matchedGpu"),
- matchedDeviceId = jsonResponse.getInt("matchedDeviceId")
- )
+ // Evict oldest entries if cache is full
+ if (cache.size >= MAX_CACHE_SIZE) {
+ val keysToRemove = cache.keys().toList().take(cache.size - MAX_CACHE_SIZE + 1)
</file context>
Suggested change
| // Evict oldest entries if cache is full | |
| // Evict arbitrary entries if cache is full (ConcurrentHashMap has no insertion order) |
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
1 participant
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 stability issues (HTTP leaks, thread safety, and missing fallback configs) and improves performance in rendering and caching. This reduces connection pool exhaustion, avoids crashes, and speeds up GPU lookups.
Bug Fixes
Performance
Written for commit be4f843. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Performance Improvements