Codechange: Replace path cache deques with vectors. #12345
Conversation
PeterN
changed the title
PeterN
force-pushed
the
path-cache-vector
branch
2 times, most recently
from
a4d827c
to
b43cff1
Compare
PeterN
force-pushed
the
path-cache-vector
branch
from
b43cff1
to
ff9f086
Compare
PeterN
changed the title
| std::reverse(std::begin(rv->path), std::end(rv->path)); | ||
| } | ||
| } | ||
|
|
||
| void Save(Vehicle *v) const override |
There was a problem hiding this comment.
A reference probably makes more sense in this context, a nullptr vehicle doesn't make much sense. But I guess I lot of the code here is C-style passing pointers so in that sense it would be consistent. Whatever you prefer.
Same for the other functions btw.
There was a problem hiding this comment.
It's the interface to SaveLoad that uses pointers, I'm not adding to changing anything here.
Ship and RoadVehicle path caches use a std::deque, which is quite memory hungry, especially for RoadVehicle which has two. std::deque was used to be able to push/pop from either end. Change to use a single std::vector each, which is now push/popped from the back.
PeterN
force-pushed
the
path-cache-vector
branch
from
ff9f086
to
b4839cc
Compare
|
I also have an alternative version of this that uses a standard saveload struct list for the road vehicle path cache, which avoids having to split and merge. However the code for merging is still needed for loading existing saves... |
I think the "standard" way would be best in the long term. If we ever were to change/add something, we don't have to reinvent the conversion if you've already have implemented that. |
Motivation / Problem
RoadVehicle path cache has two std::deques which is quite awkward to manage, and also quite memory hungry.
Ship path cache use a single std::deque.
std::deque was used to be able to push/pop from either end.
#12275 attempts to partially address RoadVehicle's path cache by hiding the fact it has two deques.
With this PR I address both the split, and the memory usage.
Description
Instead, change both to use a single
std::vectoreach. ForRoadVehicles this now contains astd::pair<Trackdir, TileIndex>which simplifies usage.This is now push/popped from the back, so sometimes needs to be reversed.
Also a special-case for ships that was copied across in the original implementation, which no longer exists for ships, has been removed for RVs. As far as I can tell this never made any sense, as an RV destination tile cannot have a path finding choice, so would never appear in the path cache anyway.
Limitations
Performance not compared. No regression expected for Ships, but RVs has a little bit of erasing which could perhaps be improved.
Saveload code for RoadVehicle is a bit gnarly as std::pair is difficult to saveload, so it always uses two temporary vectors. Maybe using a struct instead would help.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.