refactor: player save logic for async tasks and fmt usage#3720
refactor: player save logic for async tasks and fmt usage#3720dudantas wants to merge 3 commits intoopentibiabr:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors player save logic to enable asynchronous task scheduling while replacing stringstream-based SQL construction with fmt-based formatting. The changes modernize SQL generation, reduce memory allocations, and introduce optional async save processing without altering data schemas or protocols.
- Replaces inefficient stringstream SQL construction with fmt::format and fmt::memory_buffer
- Adds SaveManager::addTask for async save scheduling with synchronous fallback
- Improves DBInsert execution with trailing comma handling and buffer management
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/io/functions/iologindata_save_player.cpp | Converts SQL generation from stringstream to fmt, adds async task scheduling, implements proper DBInsert::execute calls |
| src/game/scheduling/save_manager.hpp | Adds addTask method declaration for async task scheduling |
| src/game/scheduling/save_manager.cpp | Implements addTask with try/catch wrapper and sync/async execution based on TOGGLE_SAVE_ASYNC |
| src/database/database.hpp | Adds getNumColumns method and m_numColumns member to DBResult |
| src/database/database.cpp | Implements getNumColumns, improves DBInsert::execute with better memory management and trailing comma handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| std::vector<std::string> columns; | ||
| const size_t expectedColumnCount = std::max<size_t>(result->getNumColumns(), 96); | ||
| columns.reserve(expectedColumnCount); |
There was a problem hiding this comment.
The magic number 96 should be replaced with a named constant that explains why this specific value is expected for the column count.
| if (!itemsQuery.execute()) { | ||
| g_logger().warn("[IOLoginData::savePlayer] - Error executing insert query 'player_items' for player: {}", player->getName()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The execute() call for itemsQuery is now present, but it was missing in the original saveItems function. However, the saveItems function no longer calls execute() internally, which means items won't be saved unless execute() is called after saveItems. This creates a dependency that could lead to data loss if not handled properly in all calling contexts.
| if (!batchValues.empty() && batchValues.back() == ',') { | ||
| batchValues.pop_back(); | ||
| } |
There was a problem hiding this comment.
This trailing comma removal logic should be moved earlier in the process to prevent the comma from being added in the first place, rather than removing it after the fact. This would be more efficient and clearer.
Replaces stringstream-based SQL query construction with fmt::format and fmt::memory_buffer for improved performance and readability. Hardens DBInsert::execute to safely join base query, batch values, and ON DUPLICATE KEY UPDATE (no trailing commas), uses g_database(), and clears internal buffers after execution. Introduces SaveManager::addTask to schedule save operations asynchronously with try/catch and a synchronous fallback via TOGGLE_SAVE_ASYNC, reducing blocking on database writes. Updates IOLoginDataSave methods to use the new async task system and modernizes query building throughout player save routines. Adds getNumColumns to DBResult for more robust column handling. Refactor player login/save logic to repository pattern Introduces repository interfaces and DB-backed implementations for player login and save operations, as well as VIP account management. IOLoginData, IOLoginDataLoad, and IOLoginDataSave now delegate to these repositories via DI helpers, improving separation of concerns and testability. Build scripts (CMake/MSVC) updated to include new sources; friend declarations added to player.hpp. Refactor column reserve logic in savePlayerFirst Uses a static constexpr minimum reserve and a pre-sized vector of SET clauses joined with “, ” for clarity and maintainability; schedules the first-phase UPDATE via SaveManager::addTask when appropriate.
dudantas
force-pushed
the
refactor-player-save
branch
from
8c77979 to
ac8dac5
Compare
Introduces test override functions for ILoginDataLoadRepository and ILoginDataSaveRepository, allowing injection of test implementations. Adds unit tests to verify IOLoginDataLoad and IOLoginDataSave correctly forward calls to their respective repositories. Updates CMakeLists to include new test sources.
|
|
This PR is stale because it has been open 45 days with no activity. |
Description
Refactor player save logic to enable async scheduling and adopt fmt-based SQL construction. Replace stringstream with fmt::format and fmt::memory_buffer across save paths for lower allocations and clearer code. Add SaveManager::addTask to dispatch DB writes asynchronously when TOGGLE_SAVE_ASYNC is true, with a synchronous fallback. Update IOLoginDataSave methods to use the new task path and to build UPDATE SET clauses via pre-sized vectors. Harden DBInsert::execute to trim trailing commas, reserve buffers, clear state after runs, and use g_database(). Add DBResult::getNumColumns with m_numColumns initialized in the constructor for safer column handling. No schema or protocol changes.
This change modernizes SQL construction and introduces optional asynchronous save scheduling.
Dependencies: fmt (already in tree), magic_enum (already in tree). No new external deps.
Behaviour
Actual
Expected
Type of change
How Has This Been Tested
Steps:
Checklist