⚡ Optimize Citra config updates to single-pass parsing#28
⚡ Optimize Citra config updates to single-pass parsing#28
Conversation
…of O(K*N) sequential regex replacements. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @Ven0m0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly optimizes the process of updating Citra configuration files. The previous method involved iterating through the entire configuration file for each individual setting change, leading to quadratic time complexity. The new implementation introduces a single-pass parsing mechanism that processes all desired updates simultaneously, drastically reducing the computational overhead and improving the responsiveness of configuration management, particularly when applying game-specific presets. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance optimization by refactoring the configuration update logic. Instead of repeatedly scanning the configuration file for each key, the new UpdateConfig function processes the file in a single pass, which is a great improvement from O(K*N) to O(N) complexity. The changes in ApplyPreset to batch updates are clean and effectively leverage the new function. I have one minor suggestion to improve the robustness of the new parsing logic.
| line := A_LoopField | ||
| pos := InStr(line, "=") | ||
| if (pos > 1){ | ||
| keyCandidate := RTrim(SubStr(line, 1, pos-1)) |
There was a problem hiding this comment.
Using RTrim here only removes whitespace from the right side of the key. If a key in the configuration file has leading whitespace (e.g., key = value), it will be parsed incorrectly as " key", and the update for "key" will be missed. Using Trim instead will handle both leading and trailing whitespace, making the parsing more robust to formatting variations in the config file.
keyCandidate := Trim(SubStr(line, 1, pos-1))
…stall. - Removed `--version=1.1.37.02` from `autohotkey.portable` install as it is no longer available in Chocolatey repos. - Removed `--version=2.0.19` from `autohotkey` install for consistency and to prevent similar future failures. - This allows the workflow to install the latest available versions. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes the Citra configuration update mechanism by replacing a multi-scan approach with a single-pass parser. The previous implementation scanned the config file once for each key update (O(K*N) complexity), while the new implementation parses the file once regardless of the number of keys being updated (O(N) complexity).
Changes:
- Replaced
SetKeyandRegExEscapefunctions with a newUpdateConfigfunction that performs single-pass parsing - Refactored
ApplyPresetto build an updates map and apply all changes in one operation - Removed unused helper functions
| keyCandidate := RTrim(SubStr(line, 1, pos-1)) | ||
| if (remaining.HasKey(keyCandidate)){ | ||
| val := remaining[keyCandidate] | ||
| line := keyCandidate "=" val | ||
| remaining.Delete(keyCandidate) | ||
| } |
There was a problem hiding this comment.
Indentation is inconsistent in this block. Lines 65, 67, 68, and 69 use extra spaces instead of the standard 2-space indentation pattern used throughout the file. This should be corrected to match the surrounding code's indentation style.
| keyCandidate := RTrim(SubStr(line, 1, pos-1)) | |
| if (remaining.HasKey(keyCandidate)){ | |
| val := remaining[keyCandidate] | |
| line := keyCandidate "=" val | |
| remaining.Delete(keyCandidate) | |
| } | |
| keyCandidate := RTrim(SubStr(line, 1, pos-1)) | |
| if (remaining.HasKey(keyCandidate)){ | |
| val := remaining[keyCandidate] | |
| line := keyCandidate "=" val | |
| remaining.Delete(keyCandidate) | |
| } |
| val := remaining[keyCandidate] | ||
| line := keyCandidate "=" val |
There was a problem hiding this comment.
The value retrieved from the updates map is not trimmed before being written to the config line. While the key is trimmed using RTrim on line 65, the value could potentially have leading or trailing whitespace that should be handled consistently. Consider whether values should also be trimmed, or if this is intentional to preserve exact values from the updates map.
| Loop, Parse, content, `n, `r | ||
| { | ||
| line := A_LoopField | ||
| pos := InStr(line, "=") | ||
| if (pos > 1){ | ||
| keyCandidate := RTrim(SubStr(line, 1, pos-1)) | ||
| if (remaining.HasKey(keyCandidate)){ | ||
| val := remaining[keyCandidate] | ||
| line := keyCandidate "=" val | ||
| remaining.Delete(keyCandidate) | ||
| } | ||
| } | ||
| newContent .= line "`n" |
There was a problem hiding this comment.
The Loop Parse delimiter n with r stripped will normalize line endings to LF only (Unix-style), but the function then appends "n" (LF) to each line. This means the output will have LF line endings regardless of what the input file had. If the original qt-config.ini uses CRLF line endings (Windows standard), this changes the file format. Consider preserving the original line ending style or explicitly using CRLF ("r`n") for Windows compatibility, as specified in the repository's coding guidelines.
…2 compiler is missing. - The `autohotkey` (v2) Chocolatey package does not consistently include `Compiler\Ahk2Exe.exe` in the expected location. - Added a check: if `$compilerV2` path does not exist, fallback to `$compilerV1` (which is installed via `autohotkey.portable` and includes `Ahk2Exe`). - `Ahk2Exe` is capable of compiling v2 scripts (or syntax checking them) even if it comes from the v1 package, as long as the v2 interpreter is present (which it is). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
…package. - The hardcoded path to `Ahk2Exe.exe` in the `autohotkey.portable` package caused failures when the file was not found. - Added a `Get-ChildItem` search to dynamically find `Ahk2Exe.exe` within the package directory. - This ensures the correct path is used even if the package structure changes (e.g., version updates). - Maintained fallback logic for v2 compiler to use the discovered v1 compiler if necessary. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
1 participant
💡 What:
SetKeyfunction (which performed a full regex scan and replace for each key update) with a newUpdateConfigfunction.UpdateConfigparses the configuration file line-by-line in a single pass, updating values from an associative array map.ApplyPresetto batch all key updates into a map and callUpdateConfigonce.SetKeyandRegExEscapehelper functions.🎯 Why:
ApplyPreset, this meant scanning the fullqt-config.inifile 8-10 times sequentially.📊 Measured Improvement:
PR created automatically by Jules for task 8098694276299336411 started by @Ven0m0