[Freeciv-Dev] Re: Profiling Civserver again

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Trent Piepho wrote:
> 
> On Thu, 26 Jul 2001, Jason Dorje Short wrote:
> >
> > I haven't tried your version.  I disagree that it is cleaner - a 9x8
> > array of numbers is neither clean nor intuitive IMO.  Additionally, the
> > overhead necessary to initialize DIR_DX/DIR_DY after the map is set up
> 
> I'm not sure what you mean by overhead.  If you're talking about CPU time,
> only a few numbers need to be set once when the map is set up; you couldn't
> even measure that with gprof.  If you talking about complexity overhead then I
> do see your point.  It's yet one more thing to be initialized.

I'm talking about complexity overhead - far more important than CPU
overhead, except in extreme cases.

Not only is it one more thing to be initialized, but because the usage
wouldn't be obvious to the casual observer it's less intuitive (at least
IMO).  With the current system, everything can be figured out just by
looking at the macro.

> One thing I didn't point out before, is that with my method you can use
> different types of map wrapping.  Currently, freeciv has a map that wraps
> around on the east and west edges, but not on the north and south edges.  It's
> sort of like a cylinder.  You could also have a flat map that doesn't wrap on
> any edge, or a spherical map that wraps on all four edges.
> 
> With the way you have it, the map wrapping is hard coded into the iterator
> macro.  In order to use a different type of map, you would either need to
> recompile the server with a new macro, or stick some kind of gross pile of
> nested if()s inside the iterator to test for every kind of wrapping style.

This is an excellent point, and worth making the change for.

> > Yes, DIR_DX2 uses a more intelligent numbering system.  However, it will
> > be difficult to upgrade to use this data instead.  It's too bad DIR_DX
> > wasn't written like this from the start.
> 
> I'm not sure it would really be that hard.  They are a lot of places that use
> DIR_DX to iterate over tiles, but most of them never actually look at the dir
> variable.  You could order the tiles any way you want and it wouldn't matter.
> There are a few places that do use the direction, but usually the value
> doesn't matter, as long as it is consistent.  The only place the actual value
> matters is where the opposite direction is found by using 7-dir instead of
> (dir+4)%8.  And a few places which have a text array of "N", "S", etc. that
> direction is looked up in.  Those should be changed to a function call anyway,
> instead of identical arrays repeated in several functions.

I'm pretty sure most or all of the loops that iterate over the direction
use the value within the loop - otherwise they should have been replaced
by adjc_iterate long since.  I do believe that fixing most of them would
be easy, but there are an awful lot of them.

> > superior to the current setup).  It might be possible to do better by
> > making the cardinality an option to the macro, since I know there are
> > some places in the code where it is handled like this.  I'm not sure how
> > many places there are that are like that, though.
> 
> Something like this:

<snip code>

Yes, that's exactly what I was thinking of.  Definitely better.  Note,
though, that although it will work with either your style of border
checks or the one I'm using now, the DIR_DX2 ordering of directions has
to be used to make skipping non-cardinal directions easy.

cartesian_adjascent_iterate (which needs to be renamed), adjc_iterate,
and adjc_dir_iterate could all be mapped to this macro.

> > circulated for a while, and many circulate indefinitely.  I think patch
> > writers therefore try to make their patches as unintrusive and simple as
> > possible so that they have a better chance of being understood and
> > applied.  I agree that this is a bad thing.  Hopefully after the release
> 
> I think it's more a matter of laziness.  People want to add their feature or
> fix one bug while dealing with as little of the code as possible.  So instead
> of trying to find consistent ways to name their functions and variables, they
> just use their favorite names.  They block copy sections of other people's
> code that do what they want without understanding how it works and get things
> wrong.

Well, that's bad :-(  I'll try to buck the trend, and clean up some of
this code.

Patch will follow...

jason