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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sat, Oct 13, 2001 at 02:17:18PM +0100, Gaute B Strokkenes wrote:
> 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.

I think (and a new profile with the CHECK_MAP_POS patch (I hope Jason
has posted one) should confirm this) is that the usage pattern are as
follows:
 - every map creation (MAPSTEP, iterare macros) calls
 normalize_map_pos (you can do optimizations like is_border)
 - input validation (currently done with check_coords at some places)
 should be done with is_normal_map_pos
 - function arguement validation is done with CHECK_MAP_POS

So in the long run I don't see a use for is_real_tile and
check_coords. Please wait till the CHECK_MAP_POS patch is applied and
than do another profile.

Overall I agree that macros are faster. And maybe normalize_map_pos
will change into one.

> >> +#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.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Python 2.0 beta 1 is now available [...]. There is a long list of new 
  features since Python 1.6, released earlier today. We don't plan on 
  any new releases in the next 24 hours."
    -- Jeremy Hylton at Slashdot