[Freeciv-Dev] Re: Corecleanup patch updates

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 02:56 AM 01/08/12 +0200, Gaute B Strokkenes wrote:
>On Sat, 11 Aug 2001, rwetmore@xxxxxxxxxxxx wrote:
>> At 08:48 AM 01/08/09 +0200, Gaute B Strokkenes wrote:
>>
>>>2) I'm still not sold on the DIR_D{X,Y}2 stuff.  What is the
>>>   advantage of using them, as opposed to the old ones?
 [...]
>Well, I think I'm convinced that they're no worse than the old ones.
>The only thing I can think about is that the old ones went through the
>tiles in an order which maximizes locality.  I have no idea whether
>this has any measurable effect on speed, though.  Incidentally, have
>you tried to profile some AI-only games with and without your patches?

The profiler is actually quite useless on my development box. All the
time counts return "0". I probably need to setup a slower machine to 
run these. But here are `time` stats I've kept for a few plateau points.

Not a direct comparison as the map and game evolution are slightly 
different. These all ran to Year 2000 (actually stopped at the 1995 save).

1.11.10

real    15m34.977s
user    2m3.620s
sys     0m0.430s

corecleanup_02 (profiled)

real    16m25.436s
user    3m28.380s
sys     0m0.680s

corecleanup_02 

real    14m53.926s
user    1m27.970s
sys     0m0.250s

corecleanup_03debug

real    15m22.872s
user    1m49.620s
sys     0m0.460s

WRAPY world, generic topology fixes

real    15m30.375s
user    2m3.380s
sys 0m0.370s

Donut world, generic topology fixes

real    15m40.701s
user    2m22.100s
sys 0m0.570s

[...]
>As I see it, the iteration macros exist to allow user code to loop
>over certain sets of tiles safely and efficiently, whitout having to
>worry about the gory details.  There are not that many of them:
>
>common/city.h:70:#define city_map_iterate(x, y) \
>common/city.h:101:#define city_radius_iterate(dx, dy) \
>common/city.h:109:#define map_city_radius_iterate(city_x, city_y, x_itr,
y_itr) \
>common/map.h:355:#define square_iterate(SI_center_x, SI_center_y, radius,
SI_x_itr, SI_y_itr)  \
>common/map.h:373:#define adjc_iterate(RI_center_x, RI_center_y, RI_x_itr,
RI_y_itr)            \
>common/map.h:395:#define adjc_dir_iterate(center_x, center_y, x_itr,
y_itr, dir_itr)           \
>common/map.h:423:#define whole_map_iterate(WMI_x_itr, WMI_y_itr)
                    \
>common/map.h:453:#define cartesian_adjacent_iterate(x, y, IAC_x, IAC_y) \
>
>In my opinion, it is not necessary to add several layers of
>abstraction beneath them.  I am doubtful as to whether this improves
>performance, and I certainly don't think that it makes the code any
>more readable.  I find the succession of macros defined in terms of
>core_iterate() and friends are rather tricky to follow (lots of magic
>constants etc.)  Also, formatting:

You might want to look at CPP output, and/or dump some of the assembler
from gdb to compare with current code. Properly organized macros are as 
tight as the best hand optimized code - and proper organization is what 
one should be doing in writing macros once as the payback multiplier is
much more cost effective.

The first 3 of this set are just flavours of a common core, with only
the last adding anything significant. Having 3 separate implementations 
with in each case subtly different algorithms seems more confusing than
having one, plus some minor interface flavours.

The 3 adjacent_iterate ones are again radically different and messy oneoff 
implementations that core_iterate plus three interface wrappers handles 
quite robustly and more efficiently. 

square_iterate and whole_map_iterate are special cases of block_iterations.
It is a pity that there wasn't a rectangular block_iteration for simplifying
the GUI code when it was written, or map_generation or anything else that
just needs to fill their private window.

These are also missing the corresponding "dir" flavours in many cases, and 
the namespace pollution from the macro variables is really ugly. There are
standards designed specifically for avoiding this. Plus if one follows a
consistent flavour for all of these there is much less likelyhood of making
mistakes in using or fixing them.

Anyway, at the user level the details are pretty much abstracted away, and
from a maintenance aspect, the layering reduces raw/repetitive lines of 
code a lot. The magic numbers input to core iterate are not really that 
magic if you look at and understand the DIR_* arrays, which is why putting 
their definition off in a remote file is bad practice. 

Header files should give one all the interface details needed to use and 
understand the general concepts of an implementation. The "C" files are 
where the grubby details of the specific implementation get filled in, and 
one shouldn't ever need to look there unless one needs to fix the actual
implementation. This is a lesson ingrained by over 30 years of programming :-)

But I am rambling ...

>I do not like your change to the map coordinate stuff.  The basic idea
>is sound, but the implementation is far too complicated.  First of
>all, it's too cumbersome to have one set of macro and a set of real
>functions that do the same thing.  We should just stick to one of the
>two.  Also, in general it does not make sense to macroise something
>just because it is possible to do so.  It only really makes sense if
>there is any time and / or readability to save.  Also, I don't like
>macros with lower-case names that do not evaluate all their arguments
>exactly once.  (I know we have a number of those in Freeciv already,
>but I'd like to change that.)

I actually agree with just about everything you are saying here.

But where this is useful is during development when one can test and
profile those things that make an impact. The USE_MAP_FUNCTIONS is
just a quickie example of some of the common ones that others can use
and a system for doing this sort of profiling trivially and sensibly, 
rather than sending random odd patches to Paul.

I would probably keep the underlying functional implementation in the
code base for the few that went into release, just in case they needed
to be resurrected. And compiling against the functions does type checking
that the macro forms gloss over a lot, so it is worth doing this once
for a release build before shipping to catch developer coding lapses.

But apart from MAP() or the map_get_tile flavours I have plus the 
normalize_map_pos and is_real_tile sets, all this is just for EXPERIMENTAL
purposes. If it isn't useful, or nobody is going to do any serious
profiling work that uses them they aren't needed :-).

>Also, I don't like the way we have lots of map_{g,s}_fubaz macros /
>functions.  You may recall that I announced my intent to do some work
>in this area some time ago and I'll be posting patches soon, but
>briefly I have one macro, MAP(), which takes coordinates x and y as
>arguments and returns a pointer to the relevant tile.  Optionally,
>this macro will bounds-check its arguments if you give the right
>option at compile time.  The plan is to re-route all tile access
>through this macro.  That way, a lot of map_set_this() and
>map_set_that() can become MAP()->whatever = something; etc.

I guess a tile pointer can be back converted to an index offset usable
for non-tile array access or other functions. It is nicer to have MAP()
do just the index computation or pass it an object if one wants that
element. Otherwise you get CIVMAP(), HMAP(), WINDOWMAP() and other
clone proliferation.

One advantage of macros over "C" routines, is that they can be layered
and one can add special bounds checks, assert conditions or the like
trivially with a few lines of code. Hacking 20 or 30 "C" functions to
get the same effect is not pleasant. Nor is mainataining the "C" functions
when you have to propagate a bug fix. The random flavours of map.c
get/sets and other utility routines attests to this. 

So I'm looking forward to your changes to clean this area up.

In C++ or most modern languages you subclass to avoid code duplication
and minimize maintenace divergences. In "C" the macro preprocessor serves
the same purpose. Avoidng its use does not make sense :-)

>You're also assuming that you can normalize x and y coordinates
>separately.  That's fine for rectangles, cylinders and tori but fails
>for Möbius strips and Klein bottles.  The last two are perfectly
>possibly, if a bit odd, choices of topology and it would be kind of
>fun to support them.

Actually I think I have said exactly the opposite in many places including
the origianl reply. is_real_tile(x,y) and normalize_map_pos(x,y) are the 2
necessary and sufficient interfaces to any coordinate transformation.
Eradicating adjust_map_* and the others that are single index is pretty 
much underway. If not this, then converting them to take both x,y args
would be good for long term changes.

The one difficulty with normalize_map_pos() is that getting a coordinate
is a multi-step process, first allocating and/or updating storage locations
then getting the status return, and finally loading the values.

Something like the side-effect clean map_adjust_x() to return a single 
coordinate still has some uses. Especially in expressions, as opposed
to code blocks. The difficulty is what do you do when it can't return
a legitimate value?

[...]
>Yes.  This is one place where I think it's better no to use
>adc_iterate(), since doing it the current way ensures that the goto
>route is checked.

If the goto route check is important, I can put the assert back into an
invalid else clause to make sure that goto is not set on bad tiles.

But since part of the purpose of the iterate macros is to make sure 
code only executes for valid cases, I'm not sure why one wants to 
regress to building back in all the old fault lines - in the absence
of a definite bug hunting goal of course :-).

Is there not a better place to check this? Like on/just after creation?

[...]
>This is right on all counts, but it doesn't change the fact that
>void_tile is a bad hack which ought to go away.  Cleanup patches
>should move us closer to that goal, and not further away.

I guess I see uses for void_tile that aren't part of what seems to be 
the only perceived purpose for it :-). If it were only a hack I would
agree with you, but being new, I don't have the past scars to make me
wince everytime one mentions it in arbitrary contexts.

>> If you look at Jason's latest flavour for these which I have
>> adopted, the main body of the loop is a "if" clause. One can do an
>> "} else {" and run an invalid clause within the loop if needed.
>
>Hm.  I don't think we want to make that a part of the documented
>interface.

For the adjacent macros since they all have a basic core flavour
making this part of the documented interface and forcing the macro
to always behave this way is actually quite a good thing. It is the
way most of the clone implementations work, and taking this away from
developers will just lead them back to making their own clones which
*does* defeat the purpose of the exercise.

>I would prefer to make MAX_COST a define somewhere.  

It already is, and the scattered 255's are all gone in favour of it.

>Reasonable people
>can disagree on how beautiful memcpy() is.  However, if we're not
>going to use void_tile we would have to use another array somewhere,
>at which point it would definitely look silly.

It should be a struct or wrapped in one, as opposed to a sub-element
like a char array. One could then do a true structure copy and let
the compiler insert the memcpy itself under the hood.

[...]
>--
>Big Gaute                               http://www.srcf.ucam.org/~gs234/
>..  bleakness....  desolation....  plastic forks...
>