Skip to content

Fix compatibility bugs and performance issues #668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gigachadmma69 wants to merge 1 commit into
base: master
Choose a base branch
from
+81 −66
Open

Fix compatibility bugs and performance issues #668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 29 additions & 27 deletions app/src/main/java/app/gamenative/utils/BestConfigService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import java.util.concurrent.ConcurrentHashMap
object BestConfigService {
private const val API_BASE_URL = "https://gamenative-best-config-worker.gamenative.workers.dev/api/best-config"
private const val TIMEOUT_SECONDS = 10L
private const val MAX_CACHE_SIZE = 100

private val httpClient = OkHttpClient.Builder()
.connectTimeout(10, TimeUnit.SECONDS)
Expand Down Expand Up @@ -108,32 +109,40 @@ object BestConfigService {

val response = httpClient.newCall(request).execute()

if (!response.isSuccessful) {
Timber.tag("BestConfigService")
.w("API request failed - HTTP ${response.code}")
return@withTimeout null
}
response.use {
if (!it.isSuccessful) {
Timber.tag("BestConfigService")
.w("API request failed - HTTP ${it.code}")
return@withTimeout null
}

val responseBody = it.body?.string() ?: return@withTimeout null
val jsonResponse = JSONObject(responseBody)

val responseBody = response.body?.string() ?: return@withTimeout null
val jsonResponse = JSONObject(responseBody)
val bestConfigJson = jsonResponse.getJSONObject("bestConfig")
val bestConfig = Json.parseToJsonElement(bestConfigJson.toString()).jsonObject

val bestConfigJson = jsonResponse.getJSONObject("bestConfig")
val bestConfig = Json.parseToJsonElement(bestConfigJson.toString()).jsonObject
val bestConfigResponse = BestConfigResponse(
bestConfig = bestConfig,
matchType = jsonResponse.getString("matchType"),
matchedGpu = jsonResponse.getString("matchedGpu"),
matchedDeviceId = jsonResponse.getInt("matchedDeviceId")
)

val bestConfigResponse = BestConfigResponse(
bestConfig = bestConfig,
matchType = jsonResponse.getString("matchType"),
matchedGpu = jsonResponse.getString("matchedGpu"),
matchedDeviceId = jsonResponse.getInt("matchedDeviceId")
)
// Evict oldest entries if cache is full
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix with Cubic

if (cache.size >= MAX_CACHE_SIZE) {
val keysToRemove = cache.keys().toList().take(cache.size - MAX_CACHE_SIZE + 1)
keysToRemove.forEach { cache.remove(it) }
}
Comment on lines +132 to +136
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


// Cache the response
cache[cacheKey] = bestConfigResponse
// Cache the response
cache[cacheKey] = bestConfigResponse

Timber.tag("BestConfigService")
.d("Fetched best config for $gameName on $gpuName (matchType: ${bestConfigResponse.matchType})")
Timber.tag("BestConfigService")
.d("Fetched best config for $gameName on $gpuName (matchType: ${bestConfigResponse.matchType})")

bestConfigResponse
bestConfigResponse
}
}
} catch (e: java.util.concurrent.TimeoutException) {
Timber.tag("BestConfigService")
Expand Down Expand Up @@ -313,7 +322,6 @@ object BestConfigService {
if (version.isNotEmpty() && !ManifestComponentHelper.versionExists(version, availableDxvk)) {
Timber.tag("BestConfigService").w("DXVK version $version not found, updating to PrefManager default")
return "DXVK $version"
filteredJson.put("dxwrapperConfig", PrefManager.dxWrapperConfig)
}
}

Expand All @@ -324,7 +332,6 @@ object BestConfigService {
if (version.isNotEmpty() && !ManifestComponentHelper.versionExists(version, availableVkd3d)) {
Timber.tag("BestConfigService").w("VKD3D version $version not found, updating to PrefManager default")
return "VKD3D $version"
filteredJson.put("dxwrapperConfig", PrefManager.dxWrapperConfig)
}
}

Expand All @@ -341,7 +348,6 @@ object BestConfigService {
if (!ManifestComponentHelper.versionExists(box64Version, box64VersionsToCheck)) {
Timber.tag("BestConfigService").w("Box64 version $box64Version not found in $containerVariant variant entries, updating to PrefManager default")
return "Box64 $box64Version"
filteredJson.put("box64Version", PrefManager.box64Version)
}
}

Expand All @@ -357,7 +363,6 @@ object BestConfigService {
if (fexcoreVersion.isNotEmpty() && !ManifestComponentHelper.versionExists(fexcoreVersion, availableFexcore)) {
Timber.tag("BestConfigService").w("FEXCore version $fexcoreVersion not found, updating to PrefManager default")
return "FEXCore $fexcoreVersion"
filteredJson.put("fexcoreVersion", PrefManager.fexcoreVersion)
}

// Validate Wine/Proton version (check separately based on container variant)
Expand All @@ -373,7 +378,6 @@ object BestConfigService {
if (!ManifestComponentHelper.versionExists(wineVersion, wineVersionsToCheck)) {
Timber.tag("BestConfigService").w("Wine version $wineVersion not found in $containerVariant variant entries, updating to PrefManager default")
return "Wine $wineVersion"
filteredJson.put("wineVersion", PrefManager.wineVersion)
}
}

Expand All @@ -399,7 +403,6 @@ object BestConfigService {
if (preset == null) {
Timber.tag("BestConfigService").w("Box64 preset $box64Preset not found, updating to PrefManager default")
return "Box64 preset $box64Preset"
filteredJson.put("box64Preset", PrefManager.box64Preset)
}
}

Expand All @@ -410,7 +413,6 @@ object BestConfigService {
if (preset == null) {
Timber.tag("BestConfigService").w("FEXCore preset $fexcorePreset not found, updating to PrefManager default")
return "FEXCore preset $fexcorePreset"
filteredJson.put("fexcorePreset", PrefManager.fexcorePreset)
}
}

Expand Down
7 changes: 4 additions & 3 deletions app/src/main/java/app/gamenative/utils/GameCompatibilityCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@ import kotlinx.serialization.encodeToString
import kotlinx.serialization.decodeFromString
import kotlinx.serialization.json.Json
import timber.log.Timber
import java.util.concurrent.ConcurrentHashMap

/**
* Persistent cache for game compatibility responses with 7-day TTL.
* Persistent cache for game compatibility responses with 6-hour TTL.
* Uses lazy expiration - checks expiration on access, not on load (optimizes performance).
*/
object GameCompatibilityCache {
private const val CACHE_TTL_MS = 6 * 60 * 60 * 1000L // 6 hours

private val inMemoryCache = mutableMapOf<String, GameCompatibilityService.GameCompatibilityResponse>()
private val timestamps = mutableMapOf<String, Long>()
private val inMemoryCache = ConcurrentHashMap<String, GameCompatibilityService.GameCompatibilityResponse>()
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

private val timestamps = ConcurrentHashMap<String, Long>()
private var cacheLoaded = false
Comment on lines +18 to 20
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


@Serializable
Expand Down
60 changes: 31 additions & 29 deletions app/src/main/java/app/gamenative/utils/GameCompatibilityService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,37 +101,39 @@ object GameCompatibilityService {

val response = httpClient.newCall(request).execute()

if (!response.isSuccessful) {
Timber.tag("GameCompatibilityService")
.w("API request failed - HTTP ${response.code}")
return@withTimeout null
}

val responseBody = response.body?.string() ?: return@withTimeout null
val jsonResponse = JSONObject(responseBody)

val result = mutableMapOf<String, GameCompatibilityResponse>()
val keys = jsonResponse.keys()
response.use {
if (!it.isSuccessful) {
Timber.tag("GameCompatibilityService")
.w("API request failed - HTTP ${it.code}")
return@withTimeout null
}

val responseBody = it.body?.string() ?: return@withTimeout null
val jsonResponse = JSONObject(responseBody)

val result = mutableMapOf<String, GameCompatibilityResponse>()
val keys = jsonResponse.keys()

while (keys.hasNext()) {
val gameName = keys.next()
val gameData = jsonResponse.getJSONObject(gameName)

val compatibilityResponse = GameCompatibilityResponse(
gameName = gameName,
totalPlayableCount = gameData.optInt("totalPlayableCount", 0),
gpuPlayableCount = gameData.optInt("gpuPlayableCount", 0),
avgRating = gameData.optDouble("avgRating", 0.0).toFloat(),
hasBeenTried = gameData.optBoolean("hasBeenTried", false),
isNotWorking = gameData.optBoolean("isNotWorking", false)
)

result[gameName] = compatibilityResponse
}

while (keys.hasNext()) {
val gameName = keys.next()
val gameData = jsonResponse.getJSONObject(gameName)

val compatibilityResponse = GameCompatibilityResponse(
gameName = gameName,
totalPlayableCount = gameData.optInt("totalPlayableCount", 0),
gpuPlayableCount = gameData.optInt("gpuPlayableCount", 0),
avgRating = gameData.optDouble("avgRating", 0.0).toFloat(),
hasBeenTried = gameData.optBoolean("hasBeenTried", false),
isNotWorking = gameData.optBoolean("isNotWorking", false)
)

result[gameName] = compatibilityResponse
Timber.tag("GameCompatibilityService")
.d("Fetched compatibility for ${result.size} games")
result
}

Timber.tag("GameCompatibilityService")
.d("Fetched compatibility for ${result.size} games")
result
}
} catch (e: java.util.concurrent.TimeoutException) {
Timber.tag("GameCompatibilityService")
Expand Down
21 changes: 17 additions & 4 deletions app/src/main/java/com/winlator/core/GPUInformation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,25 @@ public abstract class GPUInformation {
System.loadLibrary("extras");
}

// Cache parsed GPU cards JSON to avoid re-parsing on every call
private static JSONArray cachedGpuCards = null;

private static synchronized JSONArray getGpuCards(Context context) {
if (cachedGpuCards == null) {
try {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

cachedGpuCards = new JSONArray(gpuNameList);
} catch (JSONException e) {
cachedGpuCards = new JSONArray();
}
}
return cachedGpuCards;
}
Comment on lines +33 to +43
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


public static String getDeviceIdFromGPUName(Context context, String gpuName) {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
String deviceId = "";
try {
JSONArray jsonArray = new JSONArray(gpuNameList);
JSONArray jsonArray = getGpuCards(context);
for (int i = 0; i < jsonArray.length(); i++) {
JSONObject jobj = jsonArray.getJSONObject(i);
if (jobj.getString("name").contains(gpuName)) {
Expand All @@ -45,10 +59,9 @@ public static String getDeviceIdFromGPUName(Context context, String gpuName) {
}

public static String getVendorIdFromGPUName(Context context, String gpuName) {
String gpuNameList = FileUtils.readString(context, "gpu_cards.json");
String vendorId = "";
try {
JSONArray jsonArray = new JSONArray(gpuNameList);
JSONArray jsonArray = getGpuCards(context);
for (int i = 0; i < jsonArray.length(); i++) {
JSONObject jobj = jsonArray.getJSONObject(i);
if (jobj.getString("name").contains(gpuName)) {
Expand Down
3 changes: 0 additions & 3 deletions app/src/main/java/com/winlator/xserver/Drawable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,6 @@ public void drawImage(short srcX, short srcY, short dstX, short dstY, short widt

copyArea(srcX, srcY, dstX, dstY, width, height, totalWidth, this.getStride(), data, this.data);
}
this.data.rewind();
data.rewind();
forceUpdate();
}
this.data.rewind();
data.rewind();
Expand Down