[Freeciv-Dev] Re: [PATCH] is_real_tile().

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Fri, 28 Sep 2001, hawk@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> On Fri, Sep 28, 2001 at 03:14:40AM +0200, Gaute B Strokkenes wrote:
>> On Fri, 28 Sep 2001, gs234@xxxxxxxxx wrote:
>> > 
>> > This patch contains a macroised version of is_real_tile(), as
>> > discussed previously.  I do not have the means to profile this
>> > extensively ATM, but recent findings show that the run-time
>> > impact of the function call overhead is significant here.
> 
> I think it is unnecessary. As pointed out that if the code is changed
> as in my proposal we get:
>  - real functions which can be profiled

Real functions which can be profiled are only useful if the profile
can tell you something useful.  In this case, the only useful thing
that the profile tells us is that the function call overhead is very
significant, and that civserver would be much faster if we eliminated
it.  Moreover, we will not be able to squeeze any more speed out of
is_real_tile().  Either you need it or you do not, so we will not be
able to eliminate that any calls, and we are not going to find a
faster way to compute it either.  So we might as well let go.

>  - reduce the number of code pieces which hold topology knowledge by
>  one

I think that is completely immaterial.  While it is desirable to avoid
a situation where all the code has intimate knowledge of the
nitty-gritty details of the coordinate system, it makes no difference
IMNSHO if the places in map.[ch] that do have this knowledge is 5 or
10.  Another way to look at it is that normalize_map_pos() knows how
to normalise positions, and is_real_tile() knows which ones of them
that are real.

>  - a is_real_tile which is responsible for no measurable time:
>  time   seconds   seconds    calls  ms/call  ms/call  name
>  0.00     99.79     0.00    17447     0.00     0.00  is_real_tile

                              ^^^^^
/-------------------------------/
|
Only?  I get 335,734,726 calls to is_real_tile() in an autogame that
do not stem from normalize_map_pos().

That doesn't tell the whole story.  Firstly, the function call
overhead that is billed to the calling function in gprof output is
more than comparable in this case to that internal to is_real_tile().
See my other mail.

Secondly, your optimisation hinges on the fact that
normalize_map_pos() is called so much more frequently than
is_real_tile() that the cost off using normalize_map_pos() inside
is_real_tile() is outweighed by the decreased function call overhead
in normalize_map_pos().  This is an accident of circumstances and may
well change, for instance as we tidy up the code to always create
normalised coordinates so that we can get rid of many of the
normalize_map_pos() calls.

Thirdly, this optimisation gets in the way of others.  For instance,
one that I would like to try would be to do the normalisation manually
in MAPSTEP rather than calling normalize_map_pos().  That is pointless
if calling is_real_tile() right afterwards incurs the cost of
normalize_map_pos() anyway.  Off course one could do the test
manually, but then you loose on the code consolidation front anyway.

>> +#define IN_RANGE(a,b) \
>> +  (((unsigned) (a)) < ((unsigned) (b)))
>> +
> 
> No. This have to be IN_RANGE_0 or IN_RANGE_ZERO.

Why?  If I read something like 

  if (IN_RANGE(player_no, num_players))

then that has an obvious interpreation, which happens to be correct.
I do not think that 

  if (IN_RANGE_0(player_no, num_players))

or
  if (IN_RANGE_ZERO(player_no, num_players))

make it any more obvious; nor do I think it is obvious to the casual
observer that the 0 or ZERO is meant to indicate the lower bound of
the check.  But as mentioned in my other mail this point is no longer
relevant.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
PUNK ROCK!!  DISCO DUCK!!  BIRTH CONTROL!!