Skip to content

Refactor ModelPropertyContainer::m_vValues from vector to unordered_map#1302

Closed
Biendeo wants to merge 1 commit intoRetroAchievements:masterfrom
Biendeo:master
Closed

Refactor ModelPropertyContainer::m_vValues from vector to unordered_map#1302
Biendeo wants to merge 1 commit intoRetroAchievements:masterfrom
Biendeo:master

Conversation

@Biendeo
Copy link
Copy Markdown

@Biendeo Biendeo commented Mar 24, 2026

With the Wii rollout, large sets such as the Super Smash Bros. Brawl and Guitar Hero: Warriors of Rock sets have noticed significant performance overheads for people on weaker machines. While it's expected to find these limits in places, is there any performance we can claw back for sets of this size?

In this PR, I've spotted that when benchmarking demo gameplay in Guitar Hero Warriors of Rock, 6.23% of the total processing time is spent in ra::services::PrepareForPauseOnChangeEvents(), and in particular ra::data::ModelPropertyContainer::FindValue(), which is calling std::lower_bound() over a vector iterator to binary search for a corresponding value. Just eyeballing, this vector is often 11 or 12 elements large in this example.

image

This PR replaces this vector with an unordered_map which benefits from an O(1) access complexity, and has shrunk the cost of ra::services::PrepareForPauseOnChangeEvents() to 4.81% of the overall processing time. In practise this should result in an unnoticeable change in performance for smaller sets, but gets about 2-3% more performance in these larger sets.

image

@Jamiras
Copy link
Copy Markdown
Member

Jamiras commented Mar 24, 2026

Oddly enough, the data structure was changed from an std::map to a std::vector in #1264 for performance reasons.

While this solution may have marginal benefits for your specific set when using the DLL, most players won't be using the DLL and this change won't benefit them at all.

This bottleneck should be handled by not scanning all of the assets every frame. Instead, a collection of assets with their "Pause on X" behaviors enabled should be maintained across frames. I have already put an item in my backlog to address that.

@Biendeo
Copy link
Copy Markdown
Author

Biendeo commented Mar 24, 2026

I had to remind myself of this; std::map uses a red-black tree underneath, so reads would still be O(logn) plus all the overhead of navigating the tree. I can see how a vector would be quicker for reads in almost all cases here, and insertion shouldn't be an issue either given it's kept in order the whole time.

I agree with where you want to take this though, the less overhead per-frame, especially for things that haven't changed between frames, the better the performance. 👍

@Jamiras
Copy link
Copy Markdown
Member

Jamiras commented Mar 25, 2026

Please try out the changes in #1304.

@Jamiras Jamiras closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants