fix(Core/Threading): Modernize/Improve thread safety, performance, and maintainability of the MapUpdater class #21081
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ater class (#1) **1. Atomic Operations for Thread Safety** The refactored MapUpdater class now uses atomic operations (fetch_add and fetch_sub) for managing the pending_requests counter. This improves thread safety without the need for locking in certain cases, reducing contention and improving performance. Improvement: This change ensures that the modification of pending_requests can be done in a lock-free manner, which enhances concurrency and reduces the risk of race conditions. **2. Use of std::memory_order_acquire in Atomic Operations** In the refactored code, atomic operations (fetch_add and fetch_sub) are performed with the std::memory_order_acquire flag, which ensures proper synchronization between threads and avoids stale reads. Improvement: This adds stronger memory ordering guarantees, ensuring that all memory accesses before the atomic operation are properly synchronized with subsequent operations. **3. Simplified wait Method with Condition Variable Predicate** The refactored wait method has been simplified to use a condition variable predicate instead of manually checking the pending_requests count in a while loop. The new code waits until pending_requests is 0, using the atomic counter. Improvement: This enhances readability and clarity. It also ensures that the thread only wakes up when there are no pending requests, improving efficiency. **4. Introduction of schedule_task Helper Method** A new schedule_task method is introduced to centralize the logic for scheduling tasks, incrementing the pending_requests counter, and pushing tasks to the queue. This method is used by both schedule_update and schedule_lfg_update. Improvement: This reduces code duplication and makes the scheduling logic more maintainable and extensible. **5. Conditional Notification in update_finished** The refactored update_finished method uses atomic decrement and condition variable notification only when pending_requests becomes zero. This ensures that the condition variable is notified only when all tasks are complete. Improvement: This reduces unnecessary notifications, improving efficiency by avoiding spurious wakeups. **6. Thread Safety in WorkerThread** The WorkerThread function in the refactored code ensures that tasks are processed only if they are valid and the cancellation token is not set. It also handles cleanup (delete request) more clearly within the while loop. Improvement: The thread management is more robust with better handling of the cancellation condition, reducing the chances of errors during task execution. **7. Initialization of _cancelationToken** The _cancelationToken is now initialized directly in the MapUpdater constructor, ensuring it has a valid state when the object is created. Improvement: This eliminates potential issues where the cancellation token could have been used before initialization. **8. Clearer Comments and Code Organization** The refactored code includes better comments explaining the purpose of different methods, such as in deactivate, update_finished, and wait. It also better organizes logic related to task processing and synchronization. Improvement: The code is more readable and easier to understand for future developers, improving maintainability.
…ater class (#2) Improve thread safety, performance, and maintainability of the MapUpdater class **1. Atomic Operations for Thread Safety** The refactored MapUpdater class now uses atomic operations (fetch_add and fetch_sub) for managing the pending_requests counter. This improves thread safety without the need for locking in certain cases, reducing contention and improving performance. Improvement: This change ensures that the modification of pending_requests can be done in a lock-free manner, which enhances concurrency and reduces the risk of race conditions. **2. Use of std::memory_order_acquire in Atomic Operations** In the refactored code, atomic operations (fetch_add and fetch_sub) are performed with the std::memory_order_acquire flag, which ensures proper synchronization between threads and avoids stale reads. Improvement: This adds stronger memory ordering guarantees, ensuring that all memory accesses before the atomic operation are properly synchronized with subsequent operations. **3. Simplified wait Method with Condition Variable Predicate** The refactored wait method has been simplified to use a condition variable predicate instead of manually checking the pending_requests count in a while loop. The new code waits until pending_requests is 0, using the atomic counter. Improvement: This enhances readability and clarity. It also ensures that the thread only wakes up when there are no pending requests, improving efficiency. **4. Introduction of schedule_task Helper Method** A new schedule_task method is introduced to centralize the logic for scheduling tasks, incrementing the pending_requests counter, and pushing tasks to the queue. This method is used by both schedule_update and schedule_lfg_update. Improvement: This reduces code duplication and makes the scheduling logic more maintainable and extensible. **5. Conditional Notification in update_finished** The refactored update_finished method uses atomic decrement and condition variable notification only when pending_requests becomes zero. This ensures that the condition variable is notified only when all tasks are complete. Improvement: This reduces unnecessary notifications, improving efficiency by avoiding spurious wakeups. **6. Thread Safety in WorkerThread** The WorkerThread function in the refactored code ensures that tasks are processed only if they are valid and the cancellation token is not set. It also handles cleanup (delete request) more clearly within the while loop. Improvement: The thread management is more robust with better handling of the cancellation condition, reducing the chances of errors during task execution. **7. Initialization of _cancelationToken** The _cancelationToken is now initialized directly in the MapUpdater constructor, ensuring it has a valid state when the object is created. Improvement: This eliminates potential issues where the cancellation token could have been used before initialization. **8. Clearer Comments and Code Organization** The refactored code includes better comments explaining the purpose of different methods, such as in deactivate, update_finished, and wait. It also better organizes logic related to task processing and synchronization. Improvement: The code is more readable and easier to understand for future developers, improving maintainability.
github-actions
bot
added
CORE
walkline
reviewed
Jan 4, 2025
most contributors here prefer the default member initializer. So, for consistency, let's stick to that. Co-authored-by: Anton Popovichenko <walkline.ua@gmail.com>
* Update PCQueue.h correct _queueLock in Cancel() * Update PCQueue.h correct _queueLock in bool Pop(T& value) * Update MapUpdater.cpp - remove redundant comment
|
Thank you @walkline for the thorough review. I really appreciate it. The required has been committed. |
Winfidonarleyan
approved these changes
Jan 5, 2025
Nyeriah
approved these changes
Jan 5, 2025
Nyeriah
changed the title
Nyeriah
changed the title
|
Personally, I have tested this on Linux, a friend on Windows. What has been tested:
Nothing out of the ordinary has been observed. |
|
Been running all day with this. Haven't had any issues |
|
Thanks for the feedback |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Modernize/Improve thread safety, performance, and maintainability of the MapUpdater class
Changes Proposed:
1. Atomic Operations for Thread Safety
The refactored MapUpdater class now uses atomic operations (fetch_add and fetch_sub) for managing the pending_requests counter. This improves thread safety without the need for locking in certain cases, reducing contention and improving performance.
Improvement: This change ensures that the modification of pending_requests can be done in a lock-free manner, which enhances concurrency and reduces the risk of race conditions.
2. Use of std::memory_order_acquire / memory_order_release in Atomic Operations
In the refactored code, atomic operations are performed with the std::memory_order_acquire / memory_order_release flag, which ensures proper synchronization between threads and avoids stale reads.
Improvement: This adds stronger memory ordering guarantees, ensuring that all memory accesses before the atomic operation are properly synchronized with subsequent operations.
3. Simplified wait Method with Condition Variable Predicate
The refactored wait method has been simplified to use a condition variable predicate instead of manually checking the pending_requests count in a while loop. The new code waits until pending_requests is 0, using the atomic counter.
Improvement: This enhances readability and clarity. It also ensures that the thread only wakes up when there are no pending requests, improving efficiency.
4. Introduction of schedule_task Helper Method
A new schedule_task method is introduced to centralize the logic for scheduling tasks, incrementing the pending_requests counter, and pushing tasks to the queue. This method is used by both schedule_update and schedule_lfg_update.
Improvement: This reduces code duplication and makes the scheduling logic more maintainable and extensible.
5. Conditional Notification in update_finished
The refactored update_finished method uses atomic decrement and condition variable notification only when pending_requests becomes zero. This ensures that the condition variable is notified only when all tasks are complete.
Improvement: This reduces unnecessary notifications, improving efficiency by avoiding spurious wakeups.
6. Thread Safety in WorkerThread
The WorkerThread function in the refactored code ensures that tasks are processed only if they are valid and the cancellation token is not set. It also handles cleanup (delete request) more clearly within the while loop.
Improvement: The thread management is more robust with better handling of the cancellation condition, reducing the chances of errors during task execution.
7. Initialization of _cancelationToken
The _cancelationToken is now initialized directly in the MapUpdater constructor, ensuring it has a valid state when the object is created.
Improvement: This eliminates potential issues where the cancellation token could have been used before initialization.
8. Clearer Comments and Code Organization
The refactored code includes better comments explaining the purpose of different methods, such as in deactivate, update_finished, and wait. It also better organizes logic related to task processing and synchronization.
Improvement: The code is more readable and easier to understand for future developers, improving maintainability.
This PR proposes changes to:
Issues Addressed:
Changed this basically in regards to playerbots to improve performance slightly. Personally I'm seeing a ~4% less cpu usage with 3k+ bots.
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
How to Test the Changes:
Depending on hardware there can be a performance improvement on busy servers. In any case there should not be any degradation.
Known Issues and TODO List:
How to Test AzerothCore PRs
When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].
You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:
http://www.azerothcore.org/wiki/How-to-test-a-PR
REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).
For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.