[Freeciv-Dev] Re: (PR#9020) optimization of map_get_tile

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9020 >

You need to explain the problem in more detail or point out a few code 
locations with conflicts. It is not clear what the issue is for all these 
options.

But I suspect that you are trying the usual one size fits all solution for 
something that has a good chance of being resolved as a two stepper.

First, make the standard macro that takes a (reference) map parameter as its 
leading argument - effectively option 6. This is often thought of as the 
"hidden" or zeroth reference to a C++ object. Wherever possible use this as 
the standard replacement form and way to do things. If you do a proper macro 
that uses its arguments once, there will be an interesting local variable 
optimization effect that will have real performance benefits.

For the remaining inconvertible global cases, redefine map_get_tile() as a 
cover macro in the header (i.e. oneline change) which passes the appropriate 
global parameter (or any of the global forms that might have been used in a 
more restrictive local context in the appropriate local header/file) to this 
with some comment as to why this is still global and when it is  likely to 
be converted to the standard. It is actually the case, that if you do the 
cover macro, then later create a local variable or function argument that 
overrides the variable name used in any local scope, you will have localized 
the solution with no additional change.

The mapgen standalone program patch of several years back did a significant 
upgrade of all the map components to restructure them into an object form 
with accessor methods, hardcoded defaults and optional overrides. Solutions 
to most problems can be found as examples and documentation in this code.

Of course the foolish solution of renaming some variable everywhere 
throughout the code, only has value in touching as many code lines as 
possible to invalidate pending patches and hence clear the queue of work 
done by others but waiting for maintainer action. A oneline macro in the 
appropriately scoped header or code file is all that is required. Note this 
permits any or all-at-the-same-time of the replacement options below.

Cheers,
RossW
=====

Jason Short wrote:

> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9020 >
> 
> There have been discussions before about mass-inlining of functions. 
> Arnstein made a patch for map.c, and Per wanted to inline some functions 
> in city.c.
> 
> My opinion is that changing functions to macro/inline should be done in 
> a targeted way.  Rather than do it in large batches, we should pick a 
> function or two that is heavily used (based on profiling) and optimize 
> it.  (This applies to other forms of optimization, not just macro/inlining.)
> 
> According to a profile I made (which I'll attach as a comment) 
> map_get_tile accounts for 4% or more of server execution time.  Turning 
> it into a macro speeds up the server by just over 5%.  However turning 
> it into a map is tricky because this "function" accesses the global map 
> variable, which conflicts with about 10 of its users that already have 
> local map variables.
> 
> There are several ways around this.  Here's a list, in order of 
> increasing difficulty (and approximately in order of decreasing 
> goodness).  I don't particularly care which we pick.
> 
> 1.  Don't bother to change the code at all.
> 
> 2.  Use inline instead of macro.
> 
> 3.  Change all users to use "pfmap" instead of "map".
> 
> 4.  Rename global "map" as "civ_map".
> 
> 5.  Rename global "map" as "game.map".
> 
> 6.  Remove global map entirely, have only local maps.
> 
> jason