Feature/pool/pooled trails #5121
Conversation
…e type intended to replace the major linked lists.
|
one thing to add: I set up a test mod to as much as I can stress trail creation, iteration and destruction for bench marking purposes. The branch and the baseline are so close together in performance I would call it measurement error, save for the fact that in nearly every case the branch is ever so slightly faster than the baseline. The baseline code isn't really a hot spot even in my most taxing test case so it's not too surprising that it's not a significant difference, but the trend is encouraging for the possibility of performance gains when applied more broadly. |
wookieejedi
added
cleanup
| * Internal function to expand the storage list with a new object | ||
| * for use when there are no empty slots and the storage is not capped | ||
| */ | ||
| pool_index add_new(T &in){ |
There was a problem hiding this comment.
both add_new and use_free really should use perfect forwarding to allow moving objects into a pool or creating them in place.
code/globalincs/pool.h
Outdated
| iter(SCP_Pool<T> *vec_ptr, storage_p storage_ptr){ | ||
| storage = storage_ptr; | ||
| vec = vec_ptr; | ||
| }; |
There was a problem hiding this comment.
This construtor probably should not exist, or at the very least, just be an alias to iter(..., ..., 0), as it allows to create an iterator that points to an invalid element if element 0 is empty
code/globalincs/pool.h
Outdated
| SCP_Pool<T>::iter end(){ | ||
| return iter(this,&storage,(int) storage.size()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Now that we've had so much fun with this iterator, how about we do it again.
On a serious note, having a const_iterator that does basically the same except returning const references is absolutely needed here.
Both are cbegin and cend useful overloads to enable optimizations, having a SCP_Pool<T>::const_iterator begin() const; overload is cruical to allow foreaching and iterating the pool even when we just have a const reference to it, which tends to be often.
| //#include <vector> | ||
|
|
||
| using tl::optional; | ||
| struct pool_index{ |
There was a problem hiding this comment.
Can we call it SCP_pool_index, just to keep the naming consistent with the pool itself?
There was a problem hiding this comment.
for myself ergonomics overcomes consistency in this case. pool_index is already more than I'd like to be typing for this, honestly. Since it's going to be used all over the damn place in the end I'm more than willing to bend to consensus on this one, if one can be established.
EatThePath
force-pushed
the
feature/pool/pooled-trails
branch
from
e0b82cb to
2d126f0
Compare
EatThePath
force-pushed
the
feature/pool/pooled-trails
branch
from
2d126f0 to
a0a19b6
Compare
EatThePath
force-pushed
the
feature/pool/pooled-trails
branch
from
a0a19b6 to
1eb8dfb
Compare
| *************************************************************************/ | ||
| vector<gEntry<T>> storage; | ||
| vector<size_t> known_empty; | ||
|
|
There was a problem hiding this comment.
Probably needs to be !std::is_reference<T>::value, since the () operator is C++14
code/globalincs/pool.h
Outdated
| tl::optional<pool_index> get_new(){ | ||
| T n = T(); | ||
| return store(n); | ||
| }; | ||
|
|
||
| tl::optional<pool_index> get_new(T&& args){ | ||
| T n = T(std::forward<T>(args)); | ||
| return store(n); | ||
| }; |
There was a problem hiding this comment.
Close, but now you still expect someone to construct the element for you (and forwarding that), instead of forwarding the args themselves to the constructor.
Effectively, I'd expect something like:
template<typename... args_t>
tl::optional<pool_index> get_new(args_t&&... args){
return store(T(std::forward<args_t>(args)...));
};
Goober5000
added
the
Waiting for Stable
EatThePath
force-pushed
the
feature/pool/pooled-trails
branch
from
1eb8dfb to
0ca45c4
Compare
BMagnu
removed
the
Waiting for Stable
This branch is a test case for a new generational index based data structure that aims to replace our linked lists. The expected outcomes include safer references between objects, fewer stale pointer or index related issues, better iteration performance especially over long play sessions and easy switching from fixed length to variable length data structures.
This branch is not finished, but it is in a state where others may be able to usefully comment on the idioms in it and point out any missteps or missed opportunities.
The code in pool.h is definitely not done, nor very presentable. Trails.cpp contains the most examples of lower level pointer junk needing to be rewritten, and the rest show what changes have been required in routine external code having to touch trails.
I'm open to both hearing about ways this switch can be made more seamless, and also "while we're at it" ideas on ways we can improve the codebase's long term health by deviating a bit more.