Use colony for map and vehicle item storage #31406

ifreund · 2019-06-14T01:45:09Z

Summary

SUMMARY: Infrastructure "Use colony for map and vehicle item storage."

Purpose of change

As described in #31338, colony has several advantages over the current std::list, the most important being that iterators are not invalidated by erasure. This will make the item management code much more stable and has already cleaned up one nasty hack with processing activity items.

Describe the solution

Replace the std::list<item> stored in submaps and vehicle parts with a colony<item>.
Tweak the api ofitem_stack to reflect this change.
Make necessary changes to the rest of the code to reflect this
Delete item_stack::operator[] and refactor
Delete map and vehicle item access/removal functions that take an index and refactor
Comb through code to make sure iterators are used wherever possible
Rewrite invlet_test.cpp

Additional context

I've done my best to Include What I Use, but I've certainly missed some includes. Maybe I will try and get the IWYU tooling set up when this is done.

jbytheway · 2019-06-15T13:32:46Z

colony has several advantages over the current std::list, the most important being that iterators are not invalidated by erasure

That's not true. list iterator invalidation is basically exactly the same as for colony. The advantages of colony are better memory layout and fast-ish random access.

But thanks for working on this :).

jbytheway

Looking promising. This is indeed nice cleanup. Hope this wasn't too soon to do a preliminary review :).

Have you tackled/tested activity serialization yet? Didn't see anything related to that as I perused.

jbytheway · 2019-06-15T13:41:20Z

src/submap.h

        const item &get_uppermost_item() const {
-            return sm->itm[x][y].back();
+            return *std::prev( sm->itm[x][y].cend() );


I imagine this get_uppermost_item is used e.g. for map rendering. Using the last item for that made sense when newly dropped items were always put to the back, but less so now. We probably want to eventually remove this and e.g. get the largest item on the square to render instead. But not in this PR.

Yeah that's exactly what it's for.

And yeah that's for a future PR. I've also considered rewriting some of the inventory code to display alphabetically since storage on the map is no longer ordered.

src/savegame_json.cpp

src/visitable.cpp

src/pickup.cpp

src/map.h

src/map.cpp

src/json.h

ifreund · 2019-06-15T14:30:19Z

That's not true. list iterator invalidation is basically exactly the same as for colony. The advantages of colony are better memory layout and fast-ish random access.

Um, yeah may not have totally thought that one through. The meat of this PR is turning out to be not swapping in the colony but getting rid of all indexes to items on the map and vehicles.

ifreund · 2019-06-15T14:32:49Z

Have you tackled/tested activity serialization yet? Didn't see anything related to that as I perused.

Haven't yet and it works fine for now since item_location serialization still works. I will be tweaking that some before this PR is finished to use the colony get_X_from_X functions.

jbytheway · 2019-06-15T14:43:05Z

Oh, another thing: if you can, it may be worth checking memory consumption impact of these changes. An item is quite big, so a colony<item> with only one item in it might use quite a lot of memory depending on how much space it allows for expansion. If very small stacks of items are common this might be an issue.

jbytheway · 2019-06-15T16:24:56Z

src/activity_handlers.cpp

+
+    // We have a valid target, so preform the full finish function
+    // instead of just selecting the next valid target
+    act.index = false;


IS act.index a bool? It seems unlikely...

Ah, I see later that you are treating it as a bool. I can't decide whether it's clearer to assign 0 or false here. Hmm.

Yeah I agree that it's not super clean. I considered adding either a bool or an std::vector<bool> to player_activity but decided that was outside the scope of this PR

I have an aspiration to make accessing things inside player_activity cleaner in general, with some vague ideas of how to go about it. Looking at your code here makes me want to get to that, but I probably won't soon.

jbytheway · 2019-06-15T16:33:23Z

src/activity_handlers.cpp

+    item_location &target = act->targets.back();
+
+    // Corpses can disappear (rezzing!), so check for that
+    if( !target || !target->is_corpse() ) {


The operator bool on item_location is definitely doing the wrong thing in general. That's not really your fault, but this can totally end up pointing at the wrong item (I think there have been bug reports about butchery doing that already). I think the only way to fix this sensibly is with the weak_ptr idea Kevin has discussed. But that's orthogonal to your changes.

I think what it's doing is actually OK here for now. It just searches at its stored location for an item matching its stored pointer, returning true if such an item is found.

I agree theweak_ptr plan is definitely a lot cleaner and more robust and may look at implementing that once this get wrapped up.

Bear in mind that colony deliberately tries to reuse memory, so there is a good chance that there will be some different item at the same address if one is removed and another added, and it has no way to tell the difference.

Right, but I don't think there's anything we can do about it yet without the weak_ptr trick aside from hope it's not a corpse.

ifreund · 2019-06-15T19:43:58Z

the invlet tests are currently broken, will look into a fix.

ifreund · 2019-06-16T02:13:33Z

This PR is virtually complete now. The only remaining major task is a rewrite of invlet_test.cpp to account for the new indexless, unordered nature of items on the map/in vehicles.

src/game.cpp

jbytheway · 2019-06-16T20:51:58Z

src/faction_camp.cpp

-                    const item &seed = farm_map.i_at( pos ).front();
-                    if( farm_valid_seed( seed ) ) {
+                if( farm_map.furn( pos ) == f_plant_harvest ) {
+                    // Can't use item_stack::only_item() since there might be fertilizer


Are you sure this is a concern? I don't think fertilizer actually gets added as an item on the tile; it just modifies the time to mature.

On the subject of unrelated bugs, I see this is checking specifically for f_plant_harvest, whereas it probably ought to be doing something more generic now, because there are other harvestable furnitures, I think. You don't have to fix that on this PR, though.

It should be checking for the GROWTH_HARVEST flag, but it looks as though this whole thing is largely a duplicate of iexamine::harvest, and is out of date with the current state of what iexamine::harvest does.

Which belongs in a separate PR

This function led me to believe that fertilizer stays on the tile. It could also easily be outdated though.

Cataclysm-DDA/src/iexamine.cpp

Lines 2066 to 2082 in 4d34142

ret_val<bool> iexamine::can_fertilize( player &p, const tripoint &tile,

const itype_id &fertilizer )

{

if( !g->m.has_flag_furn( "PLANT", tile ) ) {

return ret_val<bool>::make_failure( _( "Tile isn't a plant" ) );

}

if( g->m.i_at( tile ).size() > 1 ) {

return ret_val<bool>::make_failure( _( "Tile is already fertilized" ) );

}

if( !p.has_charges( fertilizer, 1 ) ) {

return ret_val<bool>::make_failure(

_( "Tried to fertilize with %s, but player doesn't have any." ),

fertilizer.c_str() );

}

return ret_val<bool>::make_success();

}

And we can see it gets added here

Cataclysm-DDA/src/iexamine.cpp

Line 2106 in 4d34142

g->m.spawn_item( tile, "fertilizer", 1, 1, calendar::turn );

You're right. I did read iexamine::fertilize_plant but I missed that line.

src/gates.cpp

src/item_stack.h

ifreund · 2019-06-17T14:48:00Z

Alright, I think this is ready to go.

The new invlet test uses a hack to enable item UUIDs, but It seems to actually be more robust than what we had previously as the old test was giving false positives after #30603.

Sorry that this is so large. If needed there are some parts that I could probably break off, but I wont have time to do so for a couple days.

tests/invlet_test.cpp

ifreund · 2019-06-17T21:22:41Z

Added a comment explaining why the unique id hack was implemented the way it is using components and rebased to clean up all the commit messages before merge.

ifreund · 2019-06-18T13:00:50Z

Some rough numbers for the performance gains of map::process_active_items() with this PR applied:

Low number of active items:
Pre colony: 5.5% of game::do_turn()
Post colony: 3.2% of game::do_turn()

Lots of active items:
Pre colony: 28.7% of game::do_turn()
Post colony: 18.0% of game::do_turn()

Profiling was done while waiting with active items strewn about the map

The next target for optimizing item processing is item::has_flag() which could be made much faster by replacing the current std::set<std::string> with our enum bitset introduced in #29368

ifreund · 2019-06-18T15:51:43Z

Profiling results part 2, Memory usage:

A worst case scenario for colony is 1 item per tile as the minimum group size is 8.

I tested this case by spawning and killing 2600 moose, to create a one corpse per tile test case.

Memory usage of submap::load()is a little over 8 times higher after applying this PR.
Pre colony: 845.42KB, 0.4% of total memory usage.
Post colony: 8.06MB, 3.5% of total memory usage.

Why is it not exactly 8 times higher?

I haven't looked into the gritty details, but colony has more memory overhead than just the stored types to increase performance. For example, there is a list to keep track of which elements have been erased so those memory locations can be reused for future insertion.

What can we do to mitigate this?

The simplest option is setting the starting group size to something less than 8 (minimum of 3). I haven't looked into how this would effect performance yet, but will do some investigation.

Another option would be to store one colony<item> per submap and a colony<item *> for every maptile. This would be more optimal for memory usage, but increases code complexity and may affect performance.

Do we need to do something about this?

I'm not sure, the memory usage isn't crazy and the current setup is probably better for performance than optimizing for lower memory usage. Up to @kevingranade though really. Regardless, I'd like to address that issue (if needed) in a follow up PR assuming that this is acceptable to merge as is.

jbytheway · 2019-06-18T18:17:00Z

Thanks for testing that. Another option is to look into making the item objects smaller. The way they are currently structured is quite wasteful in most cases.

ifreund · 2019-06-18T21:55:03Z

Conflicts resolved

ZhilkinSerg · 2019-06-19T06:24:53Z

Conflicts resolved

You wish :)

colony has a better memory layout and faster random access than the previous std::list while mantaining pointer/iterator stability

The goal here is to remove all raw indexes to items on the map or in vehicles as indexes to colonies are unstable while pointers and iterators are stable. In this commit ACT_MOVE_ITEMS, ACT_PICKUP, and ACT_WEAR were refactored to use item_location with significant code cleanup.

Notably `Pickup::pick_up()` now uses item_stack iterators instead of indexes since the previous hack proved to be too fragile. Since items are now stored unordered on the map, the order of items displayed in the pickup ui is now sorted using `item::operator<()`

Indexes to a colony are not stable through insertion/erasure and should be avoided for everything except serialization

Indexes to colonies are unstable and everything has now been converted to use iterators and pointers. Also fixed a few minor style issues and typos.

"front" no longer makes sense since items in a colony are unordered. The new function gives a debug message if there is not exactly one item at the location. Also fixed a few bugs caused by previous refactoring and caught in code review

Rewrite the item management of the the invlet test to use an item unique id hack to make it more robust and functional now that items on the map are unordered. Note: this test seems to have been giving false positives since CleverRaven#30603 was merged, since the AUTO_INV_ASSIGN option was not being properly set. This only became apparent after implementing the more robust item management code with unique ids. This also fixes a few remaing calls in the tests to a removed vehicle function which still compiled due to implicit conversion of 0 to a pointer.

ifreund · 2019-06-19T09:33:24Z

You wish :)

Look again :)

After CleverRaven#31796 item processing invalidates item_locations, which causes issues with all activities using item_locations. This commit rewrites active_item_cache to store a safe_reference<item> instead of an item_stack::iterator, removing the now unnecessary active_item_set and has() functions. This also fixes a bug with active item reording after processing introduced by CleverRaven#31406 and a minor bug with the submaps_with_active_items cache.

After CleverRaven#31796 item processing invalidates item_locations, which causes issues with all activities using item_locations. This commit rewrites active_item_cache to store a safe_reference<item> instead of an item_stack::iterator, removing the now unnecessary active_item_set and has() functions. This also fixes a bug with active item reordering after processing introduced by CleverRaven#31406 and a minor bug with the submaps_with_active_items cache.

* Rework active_item_cache to use safe references After #31796 item processing invalidates item_locations, which causes issues with all activities using item_locations. This commit rewrites active_item_cache to store a safe_reference<item> instead of an item_stack::iterator, removing the now unnecessary active_item_set and has() functions. This also fixes a bug with active item reordering after processing introduced by #31406 and a minor bug with the submaps_with_active_items cache. * Make changes from code review In particular increment the iterator properly in active_item_cache::get() * Remove redundant get() calls on safe references clang-tidy told me what to do

ZhilkinSerg · 2020-06-14T20:06:04Z

src/iexamine.cpp

-            }
-
+        case HARVEST_SAP: {
+            liquid_handler::handle_liquid_from_container( *container, PICKUP_RANGE );


Looks like *container can be a nullptr and there is no check for that.

This option is only available if has_sap is true, and has_sap can only be true if a container was found.

I'm not saying this code isn't fragile and prone to break at some point, but it is currently correct I believe.

ifreund added [C++] Changes (can be) made in C++. Previously named `Code` Items / Item Actions / Item Qualities Items and how they work and interact Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style labels Jun 14, 2019

ifreund changed the title ~~Use colony for map and vehicle item storage~~ [WIP] Use colony for map and vehicle item storage Jun 14, 2019

ifreund force-pushed the colony-item-stack branch 2 times, most recently from ac90687 to bbff510 Compare June 15, 2019 09:18

jbytheway reviewed Jun 15, 2019

View reviewed changes

ifreund force-pushed the colony-item-stack branch from e0520de to 460a8c2 Compare June 15, 2019 20:12

ifreund force-pushed the colony-item-stack branch from d276aa8 to d8f0cb1 Compare June 16, 2019 09:29

jbytheway reviewed Jun 16, 2019

View reviewed changes

ifreund changed the title ~~[WIP] Use colony for map and vehicle item storage~~ Use colony for map and vehicle item storage Jun 17, 2019

jbytheway reviewed Jun 17, 2019

View reviewed changes

tests/invlet_test.cpp Outdated Show resolved Hide resolved

tests/invlet_test.cpp Outdated Show resolved Hide resolved

ifreund force-pushed the colony-item-stack branch from 41151cd to a4481bf Compare June 17, 2019 21:19

ifreund added the Code: Performance Performance boosting code (CPU, memory, etc.) label Jun 18, 2019

ZhilkinSerg self-assigned this Jun 18, 2019

ifreund mentioned this pull request Jun 18, 2019

game::butcher() ==> avatar_action::butcher() #31604

Closed

ifreund force-pushed the colony-item-stack branch from a4481bf to ae9ae28 Compare June 18, 2019 21:54

ifreund added 8 commits June 19, 2019 11:32

Use colony for map and vehicle item storage

9d9c13e

colony has a better memory layout and faster random access than the previous std::list while mantaining pointer/iterator stability

Use item_location for butchery and disassembly

50ff098

Remove item_stack subscript operator

a2d8c5e

Indexes to a colony are not stable through insertion/erasure and should be avoided for everything except serialization

Remove map and vehicle functions using item indexes

04c1ffd

Indexes to colonies are unstable and everything has now been converted to use iterators and pointers. Also fixed a few minor style issues and typos.

ifreund force-pushed the colony-item-stack branch from ae9ae28 to 3922cca Compare June 19, 2019 09:33

kevingranade merged commit 85f057a into CleverRaven:master Jun 20, 2019

ZhilkinSerg removed their assignment Jun 20, 2019

ifreund deleted the colony-item-stack branch June 20, 2019 10:02

This was referenced Jun 20, 2019

fix item_location unpacking #31646

Merged

Fix keg iexamine with bathtubs #31652

Merged

Cataclysm can try to butcher the wrong item and prints errors if auto-butchering is in use #31446

Open

ifreund mentioned this pull request Jun 26, 2019

Rework active_item_cache to use safe references #31873

Merged

ZhilkinSerg reviewed Jun 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use colony for map and vehicle item storage #31406

Use colony for map and vehicle item storage #31406

ifreund commented Jun 14, 2019 •

edited

Loading

jbytheway commented Jun 15, 2019 •

edited

Loading

jbytheway left a comment

jbytheway Jun 15, 2019

ifreund Jun 15, 2019

ifreund commented Jun 15, 2019

ifreund commented Jun 15, 2019

jbytheway commented Jun 15, 2019 •

edited

Loading

jbytheway Jun 15, 2019

jbytheway Jun 15, 2019

ifreund Jun 15, 2019

jbytheway Jun 15, 2019

jbytheway Jun 15, 2019

ifreund Jun 15, 2019

jbytheway Jun 15, 2019

ifreund Jun 15, 2019

ifreund commented Jun 15, 2019

ifreund commented Jun 16, 2019

jbytheway Jun 16, 2019

anothersimulacrum Jun 16, 2019 •

edited

Loading

ifreund Jun 16, 2019 •

edited

Loading

jbytheway Jun 17, 2019

ifreund commented Jun 17, 2019

ifreund commented Jun 17, 2019

ifreund commented Jun 18, 2019 •

edited

Loading

ifreund commented Jun 18, 2019 •

edited

Loading

jbytheway commented Jun 18, 2019

ifreund commented Jun 18, 2019

ZhilkinSerg commented Jun 19, 2019

ifreund commented Jun 19, 2019

ZhilkinSerg Jun 14, 2020

ifreund Jun 14, 2020

ifreund Jun 14, 2020

	ret_val<bool> iexamine::can_fertilize( player &p, const tripoint &tile,
	const itype_id &fertilizer )
	{
	if( !g->m.has_flag_furn( "PLANT", tile ) ) {
	return ret_val<bool>::make_failure( _( "Tile isn't a plant" ) );
	}
	if( g->m.i_at( tile ).size() > 1 ) {
	return ret_val<bool>::make_failure( _( "Tile is already fertilized" ) );
	}
	if( !p.has_charges( fertilizer, 1 ) ) {
	return ret_val<bool>::make_failure(
	_( "Tried to fertilize with %s, but player doesn't have any." ),
	fertilizer.c_str() );
	}

	return ret_val<bool>::make_success();
	}

Use colony for map and vehicle item storage #31406

Use colony for map and vehicle item storage #31406

Conversation

ifreund commented Jun 14, 2019 • edited Loading

Summary

Purpose of change

Describe the solution

Additional context

jbytheway commented Jun 15, 2019 • edited Loading

jbytheway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifreund commented Jun 15, 2019

ifreund commented Jun 15, 2019

jbytheway commented Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifreund commented Jun 15, 2019

ifreund commented Jun 16, 2019

Choose a reason for hiding this comment

anothersimulacrum Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

ifreund Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifreund commented Jun 17, 2019

ifreund commented Jun 17, 2019

ifreund commented Jun 18, 2019 • edited Loading

ifreund commented Jun 18, 2019 • edited Loading

Profiling results part 2, Memory usage:

Why is it not exactly 8 times higher?

What can we do to mitigate this?

Do we need to do something about this?

jbytheway commented Jun 18, 2019

ifreund commented Jun 18, 2019

ZhilkinSerg commented Jun 19, 2019

ifreund commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifreund commented Jun 14, 2019 •

edited

Loading

jbytheway commented Jun 15, 2019 •

edited

Loading

jbytheway commented Jun 15, 2019 •

edited

Loading

anothersimulacrum Jun 16, 2019 •

edited

Loading

ifreund Jun 16, 2019 •

edited

Loading

ifreund commented Jun 18, 2019 •

edited

Loading

ifreund commented Jun 18, 2019 •

edited

Loading