Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how settings and related data are saved by introducing a dedicated FileSaver thread that batches and debounces writes, reducing thread-creation overhead and preventing rapid repeated disk writes.
- Introduces
FileSaverutility to queue, batch, and debounce file writes on a single thread. - Replaces existing custom dispatchers in
ConfigHolder,Accounts, and other modules withFileSaver.save. - Adds shutdown hooks (
FileSaver.shutdown()) in error and application-exit paths to flush pending writes.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| HMCL/src/main/java/org/jackhuang/hmcl/util/FileSaver.java | New single-threaded file-saving queue with debounce and batch logic. |
| HMCL/src/main/java/org/jackhuang/hmcl/util/CrashReporter.java | Calls FileSaver.shutdown() to flush pending saves on crash. |
| HMCL/src/main/java/org/jackhuang/hmcl/setting/ConfigHolder.java | Swaps out old dispatchers for FileSaver.save and updates initial write logic. |
| HMCL/src/main/java/org/jackhuang/hmcl/setting/Accounts.java | Replaces account-saving dispatcher with FileSaver.save. |
| HMCL/src/main/java/org/jackhuang/hmcl/game/HMCLGameRepository.java | Updates saveVersionSetting to use FileSaver and directory creation. |
| HMCL/src/main/java/org/jackhuang/hmcl/Main.java | Adds FileSaver.shutdown() in exit(). |
| HMCL/src/main/java/org/jackhuang/hmcl/Launcher.java | Adds FileSaver.shutdown() in stop(). |
Comments suppressed due to low confidence (4)
HMCL/src/main/java/org/jackhuang/hmcl/util/FileSaver.java:37
- [nitpick] The sentinel name
SHUTDOWNis ambiguous when reading the queue; consider renaming it toSHUTDOWN_SIGNALor similar for clarity.
private static final Pair<Path, String> SHUTDOWN = Pair.pair(null, null);
HMCL/src/main/java/org/jackhuang/hmcl/game/HMCLGameRepository.java:333
- Changing the return type from
booleantovoidis a breaking API change; either preserve the original signature or update all callers and document this change.
public void saveVersionSetting(String id) {
HMCL/src/main/java/org/jackhuang/hmcl/util/FileSaver.java:35
- [nitpick] Add unit or integration tests for
FileSaverto verify concurrent queuing, debounce timing, and proper flush on shutdown.
public final class FileSaver extends Thread {
HMCL/src/main/java/org/jackhuang/hmcl/game/HMCLGameRepository.java:338
- Ensure
java.nio.file.Filesis imported or useFileUtilsconsistently to avoid compilation errors.
Files.createDirectories(file.getParent());
| if (shutdown) { | ||
| stopCurrentSaver(); | ||
| } else { | ||
| Pair<Path, String> head = queue.poll(30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Extract the 30-second timeout into a named constant (e.g. IDLE_THREAD_KEEP_ALIVE) to improve readability and make it easy to adjust this debounce period.
| Pair<Path, String> head = queue.poll(30, TimeUnit.SECONDS); | |
| Pair<Path, String> head = queue.poll(IDLE_THREAD_KEEP_ALIVE, TimeUnit.SECONDS); |
| } else { | ||
| map.put(head.getKey(), head.getValue()); | ||
| //noinspection BusyWait | ||
| Thread.sleep(200); // Waiting for more changes |
There was a problem hiding this comment.
Move the 200 ms debounce duration into a descriptive constant (e.g. BATCH_WINDOW_MS) and consider aligning it with the documented 100 ms debounce in the PR description.
| Thread.sleep(200); // Waiting for more changes | |
| Thread.sleep(BATCH_WINDOW_MS); // Waiting for more changes |
本 PR 重构了保存设置的方式。本次重构的目的是:
目前 HMCL 每次保存设置时都会创建一个新的线程,开销较大;
本 PR 使用单一线程保存所有设置,并在 30 秒内缓存该线程,这段时间内修改其他设置时无需创建新的线程,从而降低保存设置的开销。
目前如果在将设置保存到硬盘时发生了多次变化,则 HMCL 只会保存最后一次变化。但由于保存设置非常快,这种优化往往难以有明显效果,例如修改一些设置项时会同时让
Config多个字段发生变化,导致设置被保存多次。本 PR 改进了这项优化,遇到更改后不会立即将内容写入硬盘,而会等待 200 毫秒并仅写入最后一次变化。这解决了一些通过滑块设置的设置项在拖动滑块时会快速保存几十上百次配置的问题,因此这也是合并 支持修改主页背景的不透明度 #3205 的先决条件。
本 PR 涉及多线程之间的交互,需要小心地审查和测试。