Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port curve container types to std::vector #1635

Merged
merged 5 commits into from
May 8, 2024

Conversation

heinezen
Copy link
Member

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:

  • As elements in the curve are sorted by simulation time, insertions will like be at the end of the container in normal gameplay situations
  • Random access for reading elements is likely used more than insertion/removal
  • Copying the curve container, e.g. when passing the curve over thread boundaries, would put the the contigious memory layout of std::vector at an advantage over std::list

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

@heinezen heinezen added improvement Enhancement of an existing component lang: c++ Done in C++ code code quality Does not alter behavior, but beauty of our code labels Apr 14, 2024
@heinezen heinezen force-pushed the feature/keyframe_vector branch from 416c44b to 711272f Compare April 14, 2024 15:06
@heinezen heinezen marked this pull request as ready for review April 14, 2024 15:07
@heinezen heinezen force-pushed the feature/keyframe_vector branch from 711272f to 68cd698 Compare April 14, 2024 15:11
@TheJJ
Copy link
Member

TheJJ commented Apr 15, 2024

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
Copy link
Member Author

@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
Copy link
Member

TheJJ commented Apr 15, 2024

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
Copy link
Member Author

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

@TheJJ
Copy link
Member

TheJJ commented Apr 16, 2024

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

@heinezen
Copy link
Member Author

I've created a new branch in #1637 to preserve the changes from this PR. We could still switch to vector in the future :)

@heinezen heinezen closed this Apr 16, 2024
@heinezen heinezen reopened this Apr 17, 2024
@heinezen
Copy link
Member Author

After a quick detour with #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 heinezen force-pushed the feature/keyframe_vector branch from 68cd698 to a0ef6e1 Compare April 21, 2024 18:05
@heinezen heinezen added the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Apr 21, 2024
@SFTbot SFTbot removed the kevin-rebuild-pl0x instruct kevin to rebuild this pull request label Apr 21, 2024
}

private:
time::time_t timestamp = time::TIME_MIN;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not leave the time as a const member? then no function to access it is required.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is const then it is not copy assignable/constructible which becomes a problem for working with vector (or deque). I know I can write a new copy/assignment contructor but making them private looks more clean to me.

libopenage/curve/keyframe.h Outdated Show resolved Hide resolved
libopenage/curve/queue.h Outdated Show resolved Hide resolved
@heinezen heinezen force-pushed the feature/keyframe_vector branch 3 times, most recently from 74fe1f3 to 76e18a7 Compare April 29, 2024 17:11
@heinezen heinezen force-pushed the feature/keyframe_vector branch from 76e18a7 to 1ad330e Compare May 6, 2024 17:45
@heinezen heinezen force-pushed the feature/keyframe_vector branch from 1ad330e to d2349d5 Compare May 6, 2024 17:46
@heinezen
Copy link
Member Author

heinezen commented May 6, 2024

It's ready to review again!

@heinezen heinezen requested a review from TheJJ May 6, 2024 17:47
@TheJJ TheJJ merged commit 73c8dfd into SFTtech:master May 8, 2024
13 checks passed
@heinezen heinezen deleted the feature/keyframe_vector branch September 7, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Does not alter behavior, but beauty of our code improvement Enhancement of an existing component lang: c++ Done in C++ code
Projects
Development

Successfully merging this pull request may close these issues.

3 participants