Fewer submap inbounds checks by using _ib coords

[inogenous]

Summary

Performance "Fewer submap inbounds checks by using _ib coords"

Purpose of change

Improve performance by removing inbounds-checks when we have already checked before that a point is inbound.

Describe the solution

The main intention of this commit is to remove the inbounds check from map::get_nonant by forcing the caller to supply an already-inbounds-checked tripoint using an _ib type. But in order to do that, two other changes are required (in this commit):

1. Change submap coordinates from tripoint_rel_sm->tripoint_bub_sm.
2. Update calls to get_submap_at_grid to specify whether the point is already inbounds-checked or not.

These two changes are described below:

1. Change to tripoint_bub_sm

So, we want to use one of the _ib types.

Coordinates to specify a position for a submap used tripoint_rel_sm before this commit. But _rel_sm has no inbounds _ib type. It shouldn't. Saying that a relative coordinate is inbounds doesn't make sense - it only makes sense if we also specify another point to where it is relative to. One such point is the bubble reference.

This commit therefore changes coordinates that specify a submap so that they instead use tripoint_bub_sm. This is in fact a stronger statement about the coordinate type - now we're saying that the coordinate is relative to the bubble. Before, when using tripoint_rel_sm, the type did not explicitly state what it was relative to (but before, it was always implicitly relative to the bubble anyway).

_bub_sm also has an _ib type, so this commit uses tripoint_bub_sm_ib to specify coordinates for submaps where we've already inbounds-checked it before.

2. Update calls to get_submap_at_grid

The method get_submap_at_grid takes a submap coordinate and returns the submap. In some of the places where we call this method, we can be sure that the point is in fact inbounds, for example:

• When having called inbounds() just before, such as in map::get_submap_at( const tripoint & ):

  Cataclysm-DDA/src/map.h

  Lines 2369 to 2372 in a0579c0

  	inline submap *unsafe_get_submap_at( const tripoint &p ) {
  	cata_assert( inbounds( p ) );
  	return get_submap_at_grid( tripoint_rel_sm{ p.x / SEEX, p.y / SEEY, p.z } );
  	}

• When iterating xy between 0 and my_MAPSIZE, for example map::generate :

  Cataclysm-DDA/src/mapgen.cpp

  Lines 236 to 239 in a0579c0

  	for( int gridx = 0; gridx < my_MAPSIZE; gridx++ ) {
  	for( int gridy = 0; gridy < my_MAPSIZE; gridy++ ) {
  	for( int gridz = -OVERMAP_DEPTH; gridz <= OVERMAP_HEIGHT; gridz++ ) {
  	const tripoint_rel_sm pos( gridx, gridy, gridz );

This commit therefore updates the places where it is obvious that the point is already bounds-checked so that get_submap_at_grid or get_nonant is called with tripoint_bub_sm_ib (inbound).

The other overloaded version of get_submap_at_grid that takes tripoint_bub_sm still does inbounds checking.

Describe alternatives you've considered

More ideas (not implemented in this branch):

• We could add a method bool map::inbounds(tripoint_bub_NN p, tripoint_bub_NN_ib &p_ib) that assigns to p_ib if the point is in fact inbounds. That way, we could guard creation of inbound coords so that it's only the map itself that may create them.
• Iteration of all submaps is currently done like for( int x = 0; x < getmapsize(); x++ ) . Instead, we could change this to a map method that returns tripoint_range<> of inbound coords with some _ib type. This could guard creation of inbound coords closer to the map's responsibility.
• In some places we use const points as parameters, and in some places we use const & references as parameters. I think I read on some other PR what these tripoints are so small that they'd fit in a single register so that it would actually be preferable to not use references? Specifically, should it be get_nonant( const tripoint_bub_sm_ib &gridp ) (with reference) or get_nonant( const tripoint_bub_sm_ib gridp ) (without reference)?
• This commit also prepares us for making similar changes for mapsquare coords (tripoint_bub_ms_ib) and not only submaps (tripoint_bub_sm_ib) if we want.

Testing

• Have played on this branch using my savegame for at least 2 hours, including exploring new areas, combat and base management. Works. ✓
• Have explicitly tested map editing since some of the changed files were about that. Works. ✓

I honestly expected a larger performance increase from this, but I guess the most benefit will be for mapsquare bub_ms coords (later).

Before:

After:

Additional context

It's important that we verify that all compilers and all tests pass before merging this.

[PatrikLundell]

Are you sure creation/assignment of _ib variables contains any bounds checking? My impression from a brief look at it indicated it's just a "trust me" thing (i.e. assuring you've most certainly either checked the data or rigorously analyzed the code to ensure the value cannot be out of range, such as with the loops over known fixed ranges), but I can just have failed to find the code in the template mess.

Note that performing 3D iteration over tritype ranges will only work when the iteration order doesn't matter, i.e. the results are the same regardless of the order. That's usually the case, but map::generate has an exception.

I'm not up to date with processor design, but it sounds rather odd to store three 32 bit integers in a single register even if you had access to 128 bit registers (and also note that the game is run on a fairly wide range of processors, so assumptions made on their designs should be quite close to a lowest common denominator.
I wouldn't consider it impossible for some compilers to be able to pass a tritype variable via 3 registers, but C(++) is high level assembly where the coder is supposed to make the decisions compilers make for high level languages, so I wouldn't make any such assumptions.

Finally, I wouldn't spend my time on this kind of low level optimization unless there are clear indications there should be much performance to gain here. The big gains aren't made by minor optimizations, but by improving the logic to perform the same task with less work.

[akrieger]

This kind of stuff can be measured, but it's hard and sometimes the wins are not clear. See eg. #75376

I did take some time to try to optimize callsites above the code I was touching to use more _ib native functions instead of conversions, but the changes were invasive enough and I had enough trouble isolating it that I just stuck to what I had there. But the wins are real, it's just wins by lots of lead bullets and not single silver bullet.

Compiler definitely will smash 3 32bit ints into an sse register (or two 64 bit pointers) if it thinks it is valuable enough to. However avoiding references is less about doing that and more about allowing better static analysis and code folding.

[akrieger]

The big gains aren't made by minor optimizations, but by improving the logic to perform the same task with less work.

True. But it is categorically better to use _ib types (that are properly constructured) and avoiding inbounds() checks does save cpu time that is otherwise a friction over the entire codebase.

[inogenous]

The next step after this change would have been to also use _ib coords for hot paths that use mapsquare ms coords.

So there's that if anyone else want to pick this up later.