SFTtech / openage

Free (as in freedom) open source clone of the Age of Empires II engine 🚀
http://openage.dev
Other
12.51k stars 1.11k forks source link

Port curve container types to `std::vector` #1635

Closed heinezen closed 1 month ago

heinezen commented 2 months ago

So far, we have used std::list as the underlying type for curve containers. Lists are nice because they offer constant time insertions and erasure from anywhere in the data structure and have stable iterators, i.e. changing the list doesn't invaliate the iterators.

However, these properties may not be as important for our applications and std::vector could be better for performance under these assumptions:

This PR changes the underlying container type of KeyframeContainer and Queue to std::vector to show that its usage is possible with minimal changes.

TheJJ commented 2 months ago

how about std::deque? especially since we probably can't retain all events for the whole game and need to either serialize/forget them at earlier times

heinezen commented 2 months ago

@TheJJ std::deque is not contigous which might not be as great for hot code. The faster removal of elements at the front is nice but I don't know how often this would happen in a standard game compared to the other operations.

TheJJ commented 2 months ago

we can test/benchmark when it gets relevant, and we can easily swap it hopefully ^^ the deque isn't fully continuous but i think around 100 elements are, so its a mixture of the list and vector as far as i remember. but yea we can vector it for now i guess

heinezen commented 2 months ago

As long as we get rid of the list, it should be an improvement either way :D

TheJJ commented 2 months ago

then why not try the deque? :) should definitely be an improvement for the regular use-cases

heinezen commented 2 months ago

I've created a new branch in https://github.com/SFTtech/openage/pull/1637 to preserve the changes from this PR. We could still switch to vector in the future :)

heinezen commented 2 months ago

After a quick detour with https://github.com/SFTtech/openage/pull/1637 I think using vector is the better choice :D Iterator stability for std::deque is only guaranteed for insertion/erasure at the front and end, so caching would become more complicated in many situations. In comparison, the std::vector implementation uses indices which only require simple bounds checks to make sure that the cached index ist still in range.

I also found a C++ benchmark – std::vector VS std::list VS std::deque which compares the standard containers performance. Looks like vector is still not that much slower than deque, even for random inserts.

heinezen commented 1 month ago

It's ready to review again!