Make NavMap objects request sync only on demand #99646
Conversation
3ddc9d0
to
2664ee6
Compare
|
Do you have a method for testing this like a test project? |
For stability test just spawn, change, and delete (ten) thousands of Also helps when the reported debug monitor navigation section numbers make sense after every change. The Performance testing in a vacuum is mostly pointless for this PR as it was sliced from a bigger PR for async synchronisation missing the main accompanying pieces. I am piecemealing that larger PR atm with smaller PRs like here to escape the rebase hellscape as best as I can. |
|
Even if it's redundant in isolation, some performance metrics would be nice — if only to ensure that this isn't regressing current behavior. I have no real reason to believe there's been a regression, but I believe this is a practice we're wanting to employ for performance PRs moving forward |
|
In general I agree but as mentioned a performance test for this PR is mostly pointless in isolation without all the other async update related PRs that follow. If this is more about the performance label we can just remove it from this PR. It is like the theoretical discussion if adding code that starts and joins a thread costs more and causes a performance "regression". Sure it does, but that is not the point, the point is to offload things from the main thread because the single-threaded way is already no longer working. You can't do async updates while the map sync is just for-looping over everything like a donkey kong. |
There was a problem hiding this comment.
Fair enough. I'm not about to police a policy I'm not even sure exists, especially when it's mostly semantics in this context. The intent of this PR is extracting a bite-sized portion to make introducing the overall package more streamlined, and it succeeds at exactly that
Replaces brute-force sync check loop with a self-listing system where each object is responsible for its own dirty sync, requesting it on demand only.
2664ee6
to
ba5a357
Compare
Replaces brute-force sync check loop with a self-listing system where each map navobject is responsible for its own sync, requesting the sync only on demand once.
Previously each
NavMap::sync()the map looped over every single navobject on the map on the main thread. For each physics step it looped over allregions,links,agentsandobstacles, to check if any of them are dirty and needed to be synchronised. If dirty it called their sync function or whatever equivalent on the main thread in the middle of the physics step iteration.Let's just say having potentially thousands of unchanged navobjects on the map checked every single physics step and updated on the main thread was less than ideal. Doing all the more costly sync updates while blocking physics on the main thread was even worse. Who would have guessed?
This PR does not change things about the threading but it prepares the base. Even if performance was a minor incentive for this change the current system was also not long-term sustainable with more and more different navobjects added or additional sync points required for async updates. This PR mostly copies how this is handled by similar servers or classes.