feat(ab-testing): improve performance and info#1759
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds in-memory A/B test snapshot caching and swapping, uses in-memory diffs (with epsilon tolerance) instead of disk I/O when available, moves A/B UI into a Testing tab, and exposes JSON (de)serialization on State to support in-memory swaps and diff generation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AdvancedSettings UI
participant ABM as ABTestingManager
participant State as State
participant FS as FileSystem Util
UI->>ABM: Enable A/B testing
activate ABM
ABM->>State: SaveToJson() -> capture TEST snapshot (in-memory)
ABM->>State: SaveToJson() -> capture USER snapshot (in-memory)
ABM->>ABM: set hasTestSnapshot / hasUserSnapshot
deactivate ABM
loop Periodic Swap
ABM->>ABM: choose variant (TEST/USER)
ABM->>State: LoadFromJson(selected snapshot)
State->>State: apply config in-memory
end
UI->>ABM: Request config diff
activate ABM
ABM->>ABM: GetTestSnapshot / GetUserSnapshot
ABM->>FS: DiffJson(userJson, testJson, epsilon)
FS-->>ABM: SettingsDiffEntry list
ABM-->>UI: Render diff (limited preview / counts)
deactivate ABM
UI->>ABM: Clear / Disable A/B testing
activate ABM
ABM->>ABM: ClearCachedSnapshots()
deactivate ABM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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
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 |
There was a problem hiding this comment.
Pull request overview
This PR improves A/B testing performance and user experience by switching from disk-based configuration swaps to in-memory snapshots, and enhancing the UI with better information display.
Changes:
- Added in-memory caching of USER and TEST config variants to eliminate disk I/O during A/B test swaps
- Added epsilon-based filtering to ignore insignificant floating-point differences in config comparisons
- Refactored State class to support in-memory serialization via SaveToJson/LoadFromJson methods
- Moved A/B testing UI to a dedicated "Testing" tab in Advanced Settings
- Enhanced A/B testing overlay to display configuration differences between variants
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Utils/FileSystem.h | Added epsilon parameter to LoadJsonDiff for filtering insignificant float differences |
| src/Utils/FileSystem.cpp | Implemented epsilon-based comparison logic to skip near-equal numeric values |
| src/State.h | Added SaveToJson/LoadFromJson methods for in-memory config serialization |
| src/State.cpp | Refactored Load/Save to use new serialization methods, enabling in-memory config operations |
| src/Menu/AdvancedSettingsRenderer.h | Added RenderTestingSection method declaration |
| src/Menu/AdvancedSettingsRenderer.cpp | Moved A/B testing settings to new dedicated "Testing" tab |
| src/Features/PerformanceOverlay/ABTesting/ABTesting.h | Added snapshot caching members and methods for in-memory config management |
| src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp | Implemented in-memory snapshot caching and enhanced overlay with diff display |
| src/Features/PerformanceOverlay.cpp | Updated to use cached snapshots instead of disk reads for config diff display |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (1)
101-131: Critical bug: Swap logic is inverted.After toggling
usingTestConfigon line 105, the loading logic is backwards:
- When
usingTestConfigbecomestrue, it loadsuserConfigSnapshot(USER)- When
usingTestConfigbecomesfalse, it loadstestConfigSnapshot(TEST)This causes the flag to misrepresent the actual loaded configuration.
🐛 Proposed fix
usingTestConfig = !usingTestConfig; logger::info("A/B Test swap to {} (from memory snapshot)", usingTestConfig ? "Variant B (TEST)" : "Variant A (USER)"); if (usingTestConfig) { - // Swap to USER - load from memory snapshot (no disk I/O) - if (hasUserSnapshot) { - state->LoadFromJson(userConfigSnapshot); + // Swap to TEST - load from memory snapshot (no disk I/O) + if (hasTestSnapshot) { + state->LoadFromJson(testConfigSnapshot); } else { - logger::error("USER snapshot missing! Cannot swap to Variant A."); - usingTestConfig = false; // Stay on TEST + logger::error("TEST snapshot missing! Cannot swap to Variant B."); + usingTestConfig = false; // Stay on USER } } else { - // Swap to TEST - load from memory snapshot (no disk I/O) - if (hasTestSnapshot) { - state->LoadFromJson(testConfigSnapshot); + // Swap to USER - load from memory snapshot (no disk I/O) + if (hasUserSnapshot) { + state->LoadFromJson(userConfigSnapshot); } else { - logger::error("TEST snapshot missing! Cannot swap to Variant B."); - usingTestConfig = true; // Stay on USER + logger::error("USER snapshot missing! Cannot swap to Variant A."); + usingTestConfig = true; // Stay on TEST } }
🤖 Fix all issues with AI agents
In `@src/Features/PerformanceOverlay.cpp`:
- Around line 1210-1214: When abTestingManager->HasCachedSnapshots() is false
you must fall back to the original disk-based diff loading so the UI still
reports USER/TEST file differences; modify the block around
abTestingManager/HasCachedSnapshots()/GetConfigDiffEntries() to add an else
branch that invokes the existing disk-loading code path that previously
populated this->settingsDiff from the USER/TEST files (i.e., call the function
or logic that reads and builds the config diff from disk), ensuring settingsDiff
is set from disk when in-memory snapshots are unavailable.
In `@src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp`:
- Around line 334-335: The comment for MAX_CHANGES_DISPLAYED is inconsistent
with its value: update the comment or the constant so they match; either change
the comment "Show max 5 individual changes" to "Show max 10 individual changes"
or set constexpr size_t MAX_CHANGES_DISPLAYED = 5; so it aligns with the
intended behavior used in the if check (differences.size() <=
MAX_CHANGES_DISPLAYED). Refer to the symbol MAX_CHANGES_DISPLAYED and its use in
the condition differences.size() <= MAX_CHANGES_DISPLAYED when making the
change.
- Around line 162-202: The function GetConfigDifferencesForDisplay currently
calls Util::FileSystem::LoadJsonDiff(...) before the temp snapshot files are
written, which is dead/incorrect code; remove the premature LoadJsonDiff call
(the one using GetCommunityShaderPath()/ "temp_user_snapshot.json" and
"temp_test_snapshot.json" near the start of the function) so that LoadJsonDiff
is only invoked after writing userConfigSnapshot and testConfigSnapshot to
tempUserPath and tempTestPath, keeping the try/catch around the later
LoadJsonDiff intact and preserving early returns on file write failures.
In `@src/State.h`:
- Around line 84-86: Add thread-safety by protecting SaveToJson and LoadFromJson
with a mutex: introduce a class mutex (e.g., std::mutex m_mutex) and wrap the
entire bodies of SaveToJson(nlohmann::json& o_json) and LoadFromJson(const
nlohmann::json& i_json) with std::lock_guard<std::mutex> lock(m_mutex); so
accesses to globals::shaderCache, globals::menu and the class member variables
are serialized; alternatively, if you prefer caller-side locking, ensure Save()
and Load() acquire the same mutex before calling SaveToJson/LoadFromJson—use the
consistent mutex name (m_mutex) so callers and methods lock the same object.
In `@src/Utils/FileSystem.cpp`:
- Around line 236-245: Include the <cmath> header and change the numeric
comparisons to use double precision: when handling the "replace" case (the block
that reads aJson and bJson and compares against epsilon), retrieve values with
aJson.get<double>() and bJson.get<double>(), store them in double variables
(e.g., aDouble, bDouble), and call std::abs(aDouble - bDouble) < epsilon (ensure
epsilon is a double or cast accordingly); this ensures the correct overload of
std::abs from <cmath> is used and avoids float precision loss.
🧹 Nitpick comments (2)
src/State.cpp (1)
430-437:const_castusage may mask mutation of input parameter.The
const_caston lines 436 and 482 suggests thatMenu::LoadandFeature::Loadmay mutate their input JSON. If callers pass truly const JSON (e.g., from a const member), this could lead to undefined behavior.Consider either:
- Changing the signature to take non-const reference if mutation is expected
- Making a local copy if mutation is needed but const input must be supported
src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (1)
241-282: Temp file usage undermines in-memory caching goal.Both
GetConfigDifferencesForDisplay()andGetConfigDiffEntries()write snapshots to disk to use the existingLoadJsonDiffutility. This introduces disk I/O during what should be memory-only operations, and creates potential race conditions if these methods are called concurrently.Consider implementing a pure in-memory JSON diff utility, or at minimum use unique temp file names (e.g., with timestamps or UUIDs) to avoid conflicts.
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/State.h (1)
7-15: Remove duplicate<mutex>include.The header is included twice (lines 7 and 15). Remove one occurrence.
`#include` <Buffer.h> -#include <mutex> `#include` <nlohmann/json.hpp>
🧹 Nitpick comments (3)
src/Features/PerformanceOverlay/ABTesting/ABTesting.cpp (2)
12-57: Consider extracting shared JSON diff logic to avoid duplication.The
DiffSnapshotshelper duplicates the core diffing logic fromUtil::FileSystem::LoadJsonDiff. Both functions perform:
- JSON patch generation via
nlohmann::json::diff- Epsilon-based numeric comparison
- Replace/add/remove operation handling
Consider refactoring to share this logic, e.g., by extracting a common helper that operates on
nlohmann::jsonobjects directly, which bothLoadJsonDiff(for file-based) andDiffSnapshots(for in-memory) can use.
257-267: Consider logging cleanup failures for debugging.The catch-all block silently swallows exceptions. While this is acceptable for cleanup operations, logging a warning could help diagnose issues if snapshot clearing fails unexpectedly.
void ABTestingManager::ClearCachedSnapshots() { try { testConfigSnapshot.clear(); userConfigSnapshot.clear(); hasTestSnapshot = false; hasUserSnapshot = false; - } catch (...) { - // No-op if clear fails + } catch (const std::exception& e) { + logger::warn("Failed to clear cached snapshots: {}", e.what()); + } catch (...) { + logger::warn("Failed to clear cached snapshots: unknown error"); } }src/State.h (1)
295-295: Consider makingm_mutexprivate with accessor.The mutex is declared public, which allows external code to lock it directly. While this provides flexibility, it could lead to misuse (e.g., forgetting to unlock, inconsistent locking patterns). Consider making it private and providing a lock accessor if external locking is needed.
However, if external locking by
ABTestingManageris intentional for coordinated access, the current design is acceptable.
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
resolve ai issues (looks like they are fixed already)
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.