[Freeciv-Dev] Re: Profiling Civserver again

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

At 12:19 AM 01/07/30 -0700, Trent Piepho wrote:
>On Mon, 30 Jul 2001, Jason Dorje Short wrote:
>> "Ross W. Wetmore" wrote:
>> > 
>> > Attached is a quick hack of my accumulated macro patches into map.c,
map.h
>> > for yesterday's CVS.
>> 
>> I'm not sure how to fit these together, but I have made changes to
>> current CVS that do the following:
>
>I didn't sturdy Ross's system extensively, but isn't it an extension of the
>system I outlined?  He added the neighbor cells within a radius of 2 by
>assigning them directions after 7.  It seems like you can use the same code,
>except now you can iteratore over a radius of 2 by using directions 0 to 24,
>or just the outer ring of tiles by using directions 8 to 24.
>
>I suppose it matters if his enumeration has the nice properties the DX2 one
>has.  One property is that you can do the cardinal diections by incrementing
>the direction by two each time.  That also means that you can test if a
>direction is cardinal by just looking to see if it is odd/even.  Another
>property is that it is easy to calculate the reverse of a direction,
>(dir+4)&7.
>
>I suppose that since there are only 24 (or 25, does the center tile get
>included somewhere?) directions, is_cardinal and reverse can just be easily
>done with lookup tables.

Yes it is pretty much along the lines that have been suggested - great
minds always think alike, or is it come to the same conclusions :-).

I concatenated the 0'th, 1st and 2nd rings into a 25 element vector so
you can do fast square_iterate or border operations by picking the start
and end points. I chose the DIR_DX2-ish ordering, i.e. counterclockwise
from the NW corner, rather than N or 12 o'clock, I forget exactly why I
shifted my original off one, but it had something to do with starting in
the middle of a side in one of my prototype uses as opposed to just 
running 4 borders in succession.

With this it is easy to work out a core macro that you just feed the
start/stop/step values, and there are a raft of examples with user
friendly names that just set up the hardcoded values.

But a user can use DIR_dx and DIR_dy themselves for a number of as yet
uncoded flavours.

This has been running on my system for a couple months now. My test is
the mapgen routines which are extensively rewritten to use these after
Thue "suggested" this would be nicer in an early version. I picked up
the border wrinkle in the last week or two to stay competitive :-).

>> - Wrote a few simple macros, functions, and variables. 
>> is_normal_map_pos(x, y) returns true iff (x, y) is in its normal
>> (canonical) form.  get_dir(x, y) returns the direction from the (x,y)
>> pair.  reverse(dir) returns the reverse of the direction [1].  DIR_NAMES
>> holds the names ("N", "NW", etc.) of each direction.  is_cardinal(dir)
>> behaves as you would expect.
>
>I can see the name reverse() clashing with something.  Maybe dir_reverse() or
>dir_rev()?
>
>Also, what about creating an enum type for directions?  That way you don't
>have code with "magic numbers" like I see now in the client.
>
>enum dir_type { NORTH = 0, NORTHEAST = 1, etc. }
>
>Would putting DIR_NAMES inside a function, const char *dir_name(enum dir_type
>dir), help with i18n?  

The other thing that is very useful is to allow the user to access the
loop variable, i.e. treat it as part of the interface, not internals
though this is technically in the grey areas. I realy do want to know
which direction I am moving, and using the index to tell me is very
useful. If an enum is used, assign it the correct values, or provide 
a translation array between the DIR_dx offsets and the enum and back.

And document this in BIG LETTERS so no one accidently optimizes it for
something :-)

>> [1]  The biggest trouble I've had was figuring out that 128>>dir was
>> really supposed to be 1<<reverse(dir).  That should have been commented!
>
>When I first saw 128>>dir in the code, it took me a few seconds to figure out
>what was going on.  Cleverness like that should get commented, so people can
>appreciate it.  Too bad C doesn't have a way to access a machine bit rotate
>instruction, because 16 rotated left by dir is the same thing as
>1<<((dir+4)&7) in one operation instead of three.
>
>> There are three advantages here: the adjc_dir_iterate macro is simpler
>> (and harder to mess up) than manual iteration.  The new directional
>> system is _much_ easier to work with.  Finally, enhancing freeciv to
>> allow different map wrapping systems will be slightly easier.
>
>It's faster too, since the adjc_* macros are more efficient than the manual
>code that is in use now.
>
>> I haven't quite finished upgrading all references to DIR_DX/DIR_DY, but
>> when I do I'll send the patch along.  Right now both server and (GTK)
>> client compile and run just fine (on an AI simulation).  Obviously, it
>> should not be applied before 1.12.0.
>
>There are a few, aren't there?  I saw a few case statements in the client
that
>used direction numbers, but I guess you caught those if the client works.

The DIR_DX2 cases were only in 2 graphical routines and work fine without
needing to be touched. I *carefully* avoided touching the DIR_DX1 and 
CART_DIR_DX ones on my first round as they were not needed in any of
the loop stuff, and this would just be (destabilizing) houscleaning 
until things were settled. They are gone from mapgen though :-).

Another "problem" area I had was the DX1 and DX2 delta macros which
allow you to move "n" steps forward or backward along the border. There
is starting to become a namespace confusion issue, which I expected 
would see me doing the sort of code currying that Jason is currently
doing to clean up, even though I generally abhor such things from a
maintenance standpoint. I think if it gets done, we should do it right
this once, and make sure there is sufficient agreement and completeness
that it dosn't need another housecleaning for a while :-).

I would like to see "1" and "2" reflect the radii rather than revision
numbers of the macro changes. Noting that a city radii of "2" is getting
close to the practical limit for most uses. Beyond this there will be
a few special cases (long sighted blimps maybe), or situations where
the radii is a variable.

One of the biggest holes still is the alternate map topography and its
effects on the macros. I've yet to see this adequately detailed for all
the cases. Switching map_adjust_x() for a continue or the reverse for
a sidewise cylinder with "y" is going to be tricky.

There are always just 4 corner and 4 side cases, with the one total map
special for a single tile border check. But doing a wrap or continue in
both directions simultaneously is the sort of thing I want to use Trent's
hardcoded decisions for rather than lots of code. But the scaling problem
here needs to be addressed, at least for the block_iterate types, i.e.
those with variable rather than (small) fixed radii. I haven't thought 
this one through.

Cheers,
RossW