Skip to content

Commit b6e4efb

Browse files
GWToolbox Botclaude
andcommitted
perf(account-inventory): load inventory files off the game thread
LoadFromFiles dispatched the JSON parse to worker tasks but then busy-waited on them (`while(...) Sleep(10)`, capped at 15s) on the game thread, so the async split bought nothing: entering a map, switching characters or toggling the module froze the game until every account file had been read. Only 4 of the pool's 20 workers were ever used (4 batches), the tasks share one FIFO queue with all other Toolbox background work, and idle workers only wake on a 100ms poll - so any backlog stalls the game thread up to the cap. Make the foreign-only reload (the map/character-change path) fire-and-forget: the worker fills a shared PendingLoad and Update() applies it once ready, so the game thread never blocks. The full load (Initialize / Delete All), whose results the caller immediately builds on, still completes synchronously. A load_mutex, held by the worker for the duration of its file reads, is acquired by Terminate() and full-file saves (via FlushPendingLoad) so the module can't be torn down or have its files rewritten/deleted while a load is still reading them. The PendingLoad is shared_ptr-owned so a superseded or post-Terminate worker finishes harmlessly into an orphan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b589449 commit b6e4efb

1 file changed

Lines changed: 69 additions & 26 deletions

File tree

GWToolboxdll/Windows/AccountInventoryWindow.cpp

Lines changed: 69 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,8 @@ namespace {
465465
static bool CheckIniDirty(InventoryFile* ini, std::filesystem::file_time_type write_time);
466466
InventoryFile* GetIni(const std::string& ini_ID, const GUID& account);
467467
void LoadFromFiles(bool only_foreign);
468+
void FlushPendingLoad();
469+
void DrainPendingLoad();
468470
void SaveToFiles(bool include_foreign);
469471
void SortSlots(ImGuiTableSortSpecs* sort_specs);
470472
std::wstring GetItemEncDescription(GW::Item* item);
@@ -1007,11 +1009,19 @@ namespace {
10071009
bool success = false;
10081010
};
10091011

1010-
struct BatchLoadState {
1011-
std::mutex mutex;
1012+
// One dispatched load. A worker thread fills `results` and sets `done`; the main
1013+
// thread applies them (in Update, or a blocking flush) once done. Held behind a
1014+
// shared_ptr so an in-flight worker never dangles if the module is torn down or a
1015+
// newer load supersedes this one mid-parse.
1016+
struct PendingLoad {
10121017
std::vector<LoadResult> results;
1013-
std::atomic<int> tasks_remaining{0};
1018+
std::atomic<bool> done{false};
10141019
};
1020+
// The worker holds this for the whole duration of its file reads; Terminate() and
1021+
// full-file saves acquire it (via FlushPendingLoad) so they can't delete or overwrite
1022+
// a file while the load is still reading it.
1023+
std::mutex load_mutex;
1024+
std::shared_ptr<PendingLoad> active_load;
10151025

10161026
// Encode the wide game string as fixed-width 4-hex code units with no separators
10171027
// (lossless, and ~20% smaller than GuiUtils::ArrayToIni's space-separated form).
@@ -1218,36 +1228,65 @@ namespace {
12181228
return;
12191229
}
12201230

1221-
const size_t batch_count = std::min<size_t>(4, to_load.size());
1222-
auto state = std::make_shared<BatchLoadState>();
1223-
state->tasks_remaining = static_cast<int>(batch_count);
1231+
// Reading + parsing the files off the game thread. A newer load simply replaces
1232+
// active_load; any superseded worker keeps its own PendingLoad alive and finishes
1233+
// harmlessly into an orphan.
12241234
const GUID captured_account = current_account;
1235+
auto pending = std::make_shared<PendingLoad>();
1236+
active_load = pending;
1237+
Resources::EnqueueWorkerTask([pending, to_load, only_foreign, captured_account] {
1238+
std::scoped_lock lock(load_mutex);
1239+
for (const auto& path : to_load)
1240+
pending->results.push_back(LoadIniFile(path, only_foreign, captured_account));
1241+
pending->done.store(true, std::memory_order_release);
1242+
});
12251243

1226-
for (size_t b = 0; b < batch_count; b++) {
1227-
std::vector<std::filesystem::path> batch;
1228-
for (size_t i = b; i < to_load.size(); i += batch_count)
1229-
batch.push_back(to_load[i]);
1230-
1231-
Resources::EnqueueWorkerTask([batch, only_foreign, captured_account, state] {
1232-
for (const auto& path : batch) {
1233-
auto r = LoadIniFile(path, only_foreign, captured_account);
1234-
std::scoped_lock lock(state->mutex);
1235-
state->results.push_back(std::move(r));
1236-
}
1237-
--state->tasks_remaining;
1238-
});
1239-
}
1240-
1241-
const clock_t load_start = TIMER_INIT();
1242-
while (state->tasks_remaining > 0 && TIMER_DIFF(load_start) < 15000)
1243-
Sleep(10);
1244+
// A foreign-only reload (map/character change, module toggle) is applied later in
1245+
// Update(), so it never stalls the game thread - this is the load-in cost we're
1246+
// removing. A full load feeds the current account's own stored history that
1247+
// Initialize()/Delete All immediately build on, so it must complete before we return.
1248+
if (only_foreign)
1249+
needs_sorting = true;
1250+
else
1251+
FlushPendingLoad();
1252+
}
12441253

1245-
for (const auto& r : state->results)
1254+
// Apply a finished load's parsed results into the hierarchy (main thread). The caller
1255+
// must have observed active_load->done, so the worker is no longer touching results.
1256+
void ApplyPendingLoad()
1257+
{
1258+
if (!active_load) return;
1259+
for (const auto& r : active_load->results)
12461260
ApplyLoadResult(r);
1247-
1261+
active_load.reset();
12481262
needs_sorting = true;
12491263
}
12501264

1265+
// Non-blocking: apply the in-flight load if its worker has finished. Called every Update.
1266+
void DrainPendingLoad()
1267+
{
1268+
if (active_load && active_load->done.load(std::memory_order_acquire))
1269+
ApplyPendingLoad();
1270+
}
1271+
1272+
// Blocking: wait for the in-flight load to finish, then apply it. Used before tearing
1273+
// the module down or before a save that may delete/overwrite files a worker is reading.
1274+
void FlushPendingLoad()
1275+
{
1276+
const auto pending = active_load;
1277+
if (!pending) return;
1278+
const clock_t start = TIMER_INIT();
1279+
// The worker may still be queued behind other pool work; wait for it to publish.
1280+
while (!pending->done.load(std::memory_order_acquire) && TIMER_DIFF(start) < 15000)
1281+
Sleep(5);
1282+
// Acquiring load_mutex guarantees the worker has released the files it read. It sets
1283+
// `done` before unlocking, so a released mutex implies done; if it never ran (pool
1284+
// starved past the timeout) done stays false and we leave it for a later Update.
1285+
std::scoped_lock lock(load_mutex);
1286+
if (pending->done.load(std::memory_order_acquire))
1287+
ApplyPendingLoad();
1288+
}
1289+
12511290
// Write (or delete) the JSON file for one account.
12521291
void SyncAccountFile(const std::string& ini_ID, const GUID& account)
12531292
{
@@ -1285,6 +1324,7 @@ namespace {
12851324
void SaveToFiles(bool include_foreign)
12861325
{
12871326
if (include_foreign) {
1327+
FlushPendingLoad(); // this rewrites/deletes every file, incl. ones a load may be reading
12881328
// sync every account we know a file for (used by "Delete All": accounts is
12891329
// empty by then, so all files are removed). Snapshot first - SyncAccountFile
12901330
// mutates ini_by_path/ini_by_character.
@@ -1569,6 +1609,8 @@ void AccountInventoryWindow::Initialize()
15691609

15701610
void AccountInventoryWindow::Terminate()
15711611
{
1612+
FlushPendingLoad(); // don't tear down state (or delete the file) while a worker is reading it
1613+
active_load.reset();
15721614
GW::UI::RemoveUIMessageCallback(&OnUIMessage_HookEntry);
15731615
accounts.clear();
15741616
inventory_lookup.clear();
@@ -1776,6 +1818,7 @@ void ItemReroller::Update()
17761818

17771819
void AccountInventoryWindow::Update(float)
17781820
{
1821+
DrainPendingLoad(); // apply a finished background load without blocking the game thread
17791822
inventory_scan.Update();
17801823
item_reroll.Update();
17811824
if (save_dirty_inventories_timer && !inventory_dirty.empty() && TIMER_DIFF(save_dirty_inventories_timer) > SAVE_DIRTY_INVENTORIES_TIMEOUT) {

0 commit comments

Comments
 (0)