Better handling of item templates

[mugling]

Fixes #19566

Testing shows that removing pointer indirection from #19058 provides a significant performance benefit (16386ms to 12824ms) for the unit test described in #19566. This PR separates the fixed and runtime templates as they have differing storage requirements so that you only pay for what you need. The changes are relatively unobtrusive (59cbb24)

A considerable performance increase (12824ms to 1684ms) is provided by a change from std::map to std::unordered_map in 3d886fb

[mutability]

I am still uncomfortable about returning long-lived pointers into the internal storage of an (unordered_)map.

Can we maybe put in a "freeze" step which is called when the static templates are fully populated and

(a) refuse to hand out pointers before the freeze step is done
(b) refuse to modify the static templates after the freeze step is done

Alternatively just have both maps use unique_ptr (you can still hand out raw pointers; the change is that adding to the map won't invalidate the raw pointer if the object is being managed by a unique_ptr)

[mugling]

Can we maybe put in a "freeze" step which is called when the static templates are fully populated and

That conditional could easily be implemented in Item_factory::finalize() providing it doesn't affect performance too badly.

Alternatively just have both maps use unique_ptr

This comes with a relatively significant performance overhead which is important since anything that converts an itype_id uses this function including item construction, group lookup, ammo handling, vehicle parts etc.

The performance overhead probably has more to do with cache coherency than the actual indirection so perhaps the above conditional will be fast enough. It's tempting to suggest an assertion instead that gets excluded in the release build - this should catch any nasty bugs during development.

Overall we don't do error handling well. I'm not sure how everyone feels about exceptions but we should certainly make more use of assertions as opposed to if( !foo ) { debugmsg( ... ) } idioms. We also need to consider performance more than we have historically. In this case profiling and testing shows an obvious performance benefit so the next step could be a free step plus assertion in debug builds that should catch any problems during loading.

[mutability]

Sounds like there is scope for storing the result of item::find_type() in callers (e.g. vehicle parts seem like an obvious win). It'd be useful to look at the dynamic call graph for find_type, I'm sure it'll be 2-3 callers that account for almost all of the cost.

[mugling]

Sounds like there is scope for storing the result of item::find_type() in callers

Yes, we already do this optimisation for item. This won't work in all situations though so keeping this lookup quick remains important. Caching can bring it's own subtle bugs too.

I'm leaning towards a conceptually simple assert catching any lookups requested prior to finalize

[mutability]

Yeah, that assert would be good (plus enforcing no-modifications-after-finalize, which shouldn't be an issue to always do)

Really need to look at the profile to work out where any caching would help. While there is some lowhanging fruit here (unordered_map et al) any further gains are going to be about doing less work - call find_type() less often.

[mugling]

There is also the (unproven) possibility that keeping all the itypes in the same space might improve cache locality improving the performance of itype and it's dependent members overall. This may explain why that pointer indirection seems to matter far more than it should. If we're not careful we may add a further huge but immeasurable performance loss. I'm not sure if I have the exact terminology correct here but I should think you can comprehend the point I'm trying to make.

[mugling]

plus enforcing no-modifications-after-finalize, which shouldn't be an issue to always do

m_templates is already private and therefore short of writing an easily subverted private acessor this is already de facto the case

[mutability]

It is exactly things with access to m_templates that need to enforce the no-modification rule. For example find_template() will currently modify m_templates if you ask for an undefined ID. This is Bad and will cause hard-to-find bugs. Either that needs to assert if it happens post-freeze, or you need to put the placeholder type in the dynamic map not m_templates (and m_templates can probably lose the mutable in that case..)

The other enforcement point I see is load_basic_info which should never be called post-freeze anyway so let's enforce that obviously rather than having a subtle bug if it ever happens.

[mugling]

I'm going to add the suggested assert to this now and also fix missing items to use a runtime (not static) type

[mugling]

Note this isn't a buildbot failure - it just updates the Makefile so that RELEASE builds include the NDEBUG macro encouraging liberal usage of assert without regards to performance.

[mugling]

Final commit changes missing items to be created as runtime not static types. I think we should probably better handle missing items overall (refuse to create them) but that can come in a separate PR.

No further changes

[mutability]

I think the NDEBUG change needs to be a separate thing.

[mugling]

I think the NDEBUG change needs to be a separate thing.

No. it needs to go in this once or I'm dropping the assert

[mutability]

also, can you fix your system clock / timezone? Having commits appear in the future is pretty confusing when looking at the issue in sequence.

[mutability]

Setting NDEBUG turns off asserts everywhere. Have you checked that none of them anywhere have side-effects? That would be a bug, but it is also why I object to setting NDEBUG without care.

[mugling]

also, can you fix your system clock / timezone? Having commits appear in the future is pretty confusing when looking at the issue in sequence.

Yes, after this merge run

[mugling]

Setting NDEBUG turns off asserts everywhere. Have you checked that none of them anywhere have side-effects? That would be a bug, but it is also why I object to setting NDEBUG without care.

There aren't many in the project so I'm going to check for side effects now

$ grep 'assert' src/* | cut -d: -f1 | sort | uniq -c                                                                                             
   1 Binary file src/item_factory.o matches
   5 src/activity_item_handling.cpp
  37 src/advanced_inv.cpp
   1 src/catalua.cpp
   5 src/debug.cpp
   1 src/enums.h
   2 src/game.cpp
   1 src/inventory_ui.cpp
   1 src/item.cpp
   2 src/item_factory.cpp
   2 src/item_group.cpp
   1 src/iuse_software_snake.cpp
   1 src/line.cpp
   2 src/map.cpp
   1 src/mapgen.cpp
   1 src/mapgenformat.cpp
   4 src/mapgenformat.h
   2 src/newcharacter.cpp
   3 src/overmap.cpp
   4 src/overmapbuffer.cpp
   2 src/scent_map.cpp
   9 src/units.h
   2 src/veh_interact.cpp
   1 src/vehicle.cpp
   1 src/wdirent.h

[Coolthulhu]

No asserts in release isn't a good idea.
Line asserts are a great example - if not for the asserts, we wouldn't find them unless someone built a debug and noticed why things are all going wrong.

We need feedback from users, not just dedicated testers.

[mugling]

No side effects:

$ grep 'assert(' src/*.cpp | cut -d: -f2 | sort | uniq -c                                                                                        
   1                         assert(!elem.is_null());
   1                     assert(!moving_item.is_null());
   1                     assert(msg != nullptr);
   1                 assert( d1.cat != nullptr );
   1                 assert( d2.cat != nullptr );
   1                 assert( fuel->ammo->energy > 0 ); // enforced in item_factory.cpp
   1                 assert( g->u.has_activity( activity_id( "ACT_ADV_INVENTORY" ) ) );
   1                 assert( pt->base.max_damage() > 0 ); // why repairing part that cannot be damaged?
   1             // assert( false );
   1             // assert( type == PLTYPE_NOW );
   1             assert( !cont->contents.empty() );
   1             assert( !sitem->items.empty() );
   1             assert( &cont->contents.front() == sitem.items.front() );
   1             assert( actor );
   1             assert( amount_to_move > 0 );
   1             assert( cont != nullptr );
   1         assert( AIM_NORTHEAST - AIM_SOUTHWEST == 8 );
   1         assert( d_cargo >= 0 );
   1         assert( d_veh != nullptr );
   1         assert( g->m.inbounds( local ) );
   1         assert( ms.x >= 0 && ms.x < SEEX );
   1         assert( ms.y >= 0 && ms.y < SEEX );
   1         assert( s_cargo >= 0 );
   1         assert( s_veh != nullptr );
   1         assert( squares[menu.ret].canputitems() );
   1         static_assert( std
   1     assert( !items.empty() );
   1     assert( !items.empty() ); // index would not be valid, and this would be an endless loop
   1     assert( !sitem.items.empty() );
   1     assert( !sitem.items.empty() ); // valid item is obviously required
   2     assert( destarea != AIM_ALL ); // should be a specific location instead
   1     assert( dir != om_direction
   1     assert( div != 0 );
   1     assert( dt >= DT_NULL && dt < NUM_DT );
   1     assert( filename != nullptr );
   1     assert( funcname != nullptr );
   1     assert( get_cur_item_ptr() != nullptr ); // index must already be valid!
   1     assert( id != AIM_ALL ); // should be a specific location instead
   1     assert( input_amount > 0 ); // there has to be something to begin with
   1     assert( line != nullptr );
   1     assert( menu != nullptr );
   1     assert( menu->entries.size() >= 9 );
   4     assert( offset != 0 ); // 0 would make no sense
   2     assert( offset == -1 || offset == +1 ); // only those two offsets are allowed
   1     assert( p != invalid_tripoint );
   1     assert( sitem.area != AIM_ALL );        // should be a specific location instead
   1     assert( sitem.area != AIM_INVENTORY );  // does not work for inventory
   1     assert( square.id != AIM_ALL );
   1     assert( stacks >= 1 );
   1     assert( static_cast<size_t>( index ) < items.size() ); // valid index is required
   1     assert( w_inv );
   1     assert( zlevels );
   1     assert(!line.empty() && line.front() != line.back());
   1     assert(ptr.get() != NULL);
   1     assert(stacks >= 1);
   1  * assert(it.id == "water");

This is good because once we have NDEBUG we can start adding a lot more assert - in particular to document preconditions and the like with out concern as to performance.

[mutability]

Also, looking at the assert, it tests a boolean field. Are you seriously suggesting that testing a boolean field is sufficiently expensive that you'd rather risk unpredictable bugs? This has already broken nastily once, can we please keep the safety net here?

[mutability]

No side effects:

That is a naive assertion (heh) if you're just basing it on a grep.
Obvious things:

• There are multi-line assertions;
• You excluded any case where there's a space after the assert
• There are calls to nontrivial functions in the asserts you did list

[mugling]

No asserts in release isn't a good idea.

assert means should never happen and should only be used to effectively localize a crash during development. If something can potentially happen and/or recovered from it should be an explicit conditional plus an exception or debugmsg.

We should use assert for it's intended function otherwise we're just going to end up with real_assert macro that does have the expected characteristics.

Our current error handling is awful. We don't fail on bad JSON we just keep going. Someone even wrote a PR that swallows thrown exceptions during the process (which also eats the file and line number). The nett effect is that you get a dozen unattributed load errors followed by a segfault.

assert means should never happens and will always result in a hard crash. debugmsg means might happen and should be recovered from gracefully.

We do need an assert macro and it should actually be called assert

[mugling]

Are you seriously suggesting that testing a boolean field is sufficiently expensive that you'd rather risk unpredictable bugs?

No I'm suggesting that if they aren't compiled out they are defacto debugmsg and therefore very few assertions will be added to the codebase which is unfortunate as they are self-documenting preconditions.

[mugling]

Or put more eloquently (via stack overflow)

"If you're even thinking of leaving assertions on in production, you're probably thinking about them wrong. The whole point of assertions is that you can turn them off in production, because they are not a part of your solution. They are a development tool, used to verify that your assumptions are correct. But the time you go into production, you should already have confidence in your assumptions"

They are effectively comments that don't become outdated and make development easier by turning stopping a nullptr deference at source as opposed to allowing it to occur half a dozen function calls later. If any calls should be enabled in production those should be debugmsg

[mutability]

In this particular case:

• Callers expect the returned pointer to be stable; item_factory needs to guarantee that
• If it does not guarantee that then we have unpredictable bugs at an indeterminate later point. It is not an immediate crash

If you can't (or don't want to) enforce the stability guarantee via freezing the map then I think we need to go back to unique_ptr which does provide that guarantee.

If you think that assert() is the wrong thing to use to enforce the guarantee then that's fine, but I don't know if there's a simple way to get the same facilities (popup window so the user knows what's going on before you call abort()).

Note that debugmsg is no good here because we cannot safely continue if a lookup is done on a nonfrozen map.

[mutability]

We should use assert for it's intended function otherwise we're just going to end up with real_assert macro that does have the expected characteristics.

I would be fine with an expensive_assert that was disabled in release builds. Or we could rename existing assert calls to something else (enforce or whatever) and have plain assert be the "expensive debug-only assert" path. The call in this PR would need to be to the version that is present in release builds.

But I think that is all discussion that better belongs in a separate PR, as I originally suggested. It should not prevent this PR getting merged; drop the NDEBUG change and argue it out separately.

[mugling]

I've rebased this to everything prior to NDEBUG. Someone else confirming this works for them would be helpful so that this can close #19566

[mutability]

This needs to move up into Item_factory::all()

[mutability]

Snap. Was 3/4 of the way through the same, but let's go with yours. I found one assert that needed to be moved (see above). Also a couple of places that could use map::emplace but those aren't a big deal.

[mutability]

This could use emplace

[mutability]

This could also use emplace

[mugling]

How so?

[mugling]

I've pushed that change to assert but left the emplace out. I can't get the syntax to work and I'd be surprised if they offered any measurable performance difference

EDIT: Github really should add confirmation to 'Close and comment'

[mugling]

Merge as per above review

[mutability]

Yeah, not too worried about the emplace, it just makes it a little clearer that you are inserting a new entry and passing ownership to a unique_ptr. there's no real difference in functionality.

m_runtimes.emplace( id, std::unique_ptr<itype>( def ) );

[mutability]

release-cataclysm-tiles: src/item_factory.cpp:2301: std::vector<const itype*> Item_factory::all() const: Assertion `frozen` failed.

Rebuilding with symbols now.

[mugling]

Not sure how that escaped, I'm going to compile also

[mutability]

#4  0x0000000000727150 in Item_factory::all (this=<optimised out>) at src/item_factory.cpp:2301
#5  0x00000000006ec788 in auto_pickup::create_rules (this=this@entry=0xf3e360 <get_auto_pickup()::single_instance>) at src/auto_pickup.cpp:532
#6  0x00000000006ed115 in auto_pickup::load (this=0xf3e360 <get_auto_pickup()::single_instance>, bCharacter=bCharacter@entry=false) at src/auto_pickup.cpp:628
#7  0x00000000006ed1e1 in auto_pickup::load_global (this=<optimised out>) at src/auto_pickup.cpp:608
#8  0x00000000006aacb5 in game::load_static_data (this=this@entry=0x9c46390) at src/game.cpp:282
#9  0x000000000041e61a in main (argc=<optimised out>, argv=<optimised out>) at src/main.cpp:440

[mugling]

Note also #19240 but I'm not sure if it's relevant

[mugling]

The comments in game::load_static_data are helpful - it looks like that call should be later

[mutability]

Can probably skip initializing the ruleset after loading global rules? They're meaningless until the world data is loaded at a later point anyway. I guess it only works because the rules get regenerated when the character-specific rules get loaded.

[mugling]

I was looking at moving that call elsewhere - the code comment say should only include static data but that code attempts a JSON load?

[mugling]

Probably in game::setup after load_world_modfiles?

[mutability]

comment at the very top of load_static_data:

// Load everything that will not depend on any mods

So it seems OK to load the global autopickup config since it's just text and doesn't directly depend on any sort of moddable data, but it doesn't make sense to actually match them up to items (in create_rules()) until the world is loaded.

Delaying the loading of global rules entirely is trickier since you can edit the global rules from the settings menu with no world loaded at all.

[mugling]

Yes but see also the comments below that

    // The content they load/initialize is hardcoded into the program.
    // Therefore they can be loaded here.
    // If this changes (if they load data from json), they have to
    // be moved to game::load_mod or game::load_core_data

[mugling]

Continued in #19618