[Freeciv-Dev] Re: remove nearest_real_pos uses (PR#1211)
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Ross W. Wetmore wrote:
At 09:23 PM 02/01/08 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
Ross W. Wetmore wrote:
At 01:01 PM 02/01/07 +0100, Raimar Falke wrote:
[...]
Callers used to pass unnormal ones though, which is a soft condition that
can always be fixed. In this case normalize_map_pos() is guaranteed to
fix things and doesn't need to be checked.
So is CHECK_MAP_POS, if we implement it that way. But as long as the
soft condition is met we will be safe.
Again, if we implemented CHECK_MAP_POS as:
#ifdef DEBUG
# define CHECK_MAP_POS(x, y) assert(is_normal_map_pos(x, y))
#else
# define CHECK_MAP_POS(x, y)
if (!normalize_map_pos(&x, &y)) {
freelog(...);
nearest_real_pos(&x, &y);
}
#endif
Then we'll have exactly the same effective code, plus the additional
assertion to help us catch bugs, and it will be uniform throughout.
The effect now, is that instead of using normalize_map_pos() once at the
entry of a function or when you computed a new coordinate and were going
to make an access to something that required normal coordinates, you now
make it everytime you do such accesses which can be dozens or hundreds of
times in a routine. This is through the map_inx CHECK_MAP_POS() in
addition to all the other random ones.
You have just replaced (previously) correct local code (optimizations)
with a hidden global performance pig providing uniform de-optimization
throughout.
This statement is completely unfounded.
For the most part, CHECK_MAP_POS has simply replaced previous
invocations of normalize_map_pos. There are a few places in which this
isn't the case: for instance it has been (temporarily, I think) inserted
into the iterator macros as a debugging measure. Also the check has
been placed into map_inx, but it has been removed from map_get_tile and
most other locations that used map_inx.
If you change CHECK_MAP_POS to normalize, those parts of the code will
most likely be faster than they were before, because many spurious
normalizations have been removed. For instance, most map functions
would call normalize_map_pos, then call map_get_tile which would call
normalize_map_pos again; now there's just one check - in map_inx.
The real advantage of CHECK_MAP_POS is that it lets you choose which of
these options you want, by changing just a few lines of code in map.h.
If true safety is required, it can normalize; for pure speed it can do
nothing and assume the coordinates are normalized. Nothing has been lost.
I repeat: nothing has been lost.
Remember that normalize_map_pos() was the naughty one after warmap.
And still is, since its use has grown as we've removed map_adjust_*.
And remember this was the main argument for starting all this
CHECK_MAP_POS() activity in the beginning - the need not to have to
do these checks all over the place, just at source.
Yes, and it still is. The normalizations were (almost) all being done
at the source anyway, so this was a very logical step. The question is:
how confident are we that all these normalizations are being done?
And remember, you were warned this was neither particularly useful nor
reasonable.
It's a choice between speed and safety - you
can't have both.
There was an extensive test period for realness fixes and fair amount
of code scrutiny at one point before the corecleanups were distributed
publicly and there has been a much longer settling time for these
things there.
In that case the assertion definitely shouldn't fail - all the more
reason to put it in :-).
I do find it hilarious that Raimar needs "numbers" to permit an obviously
more efficient code construct into CVS, but the two of you repeatedly
find all these reasons to downgrade the performance with nary a whiff
of impact analysis.
It can be 50-fold increases in savegame sizes, 2% hit rates in regular
whole_map_iterate loops, going through the above cycle to increase the
number of redundant normalize_map_pos() calls in the code, replacing
array lookups with function calls in all punit access, looping over all
directions instead of computing the one you want ...
I really do have things backwards about what is important here, don't I :-?
You seem to value code efficiency over code cleanliness or readibility.
Perhaps this is because you read code better than most people, but
it's still not a good idea for an open-source project. The more
readible the code is and the more separated the interface is from the
backend, the easier it is for people to contribute.
Aside from you, AFAIK everyone who has expressed an opinion has said
that 50% hit-rates and 100% increases are acceptable for standard cases,
with higher inefficencies allowable for pathological cases. I was a bit
skeptical at first, but I quickly realized it was the easiest way to
achieve an initial implementation, and it wouldn't be too hard to
improve on it afterwards.
If this patch is applied, the only remaining nearest_real_pos use will
be in map_pos_to_canvas_pos in mapview_common. If one of these places
does need to use nearest_real_pos (which seems inconceivable), a comment
should be added explaining why.
If this function returns status, and get_canvas_xy propagates this to
its callers who then deal with the situation, this one can go.
Unfortunately no, since the only way the callers can deal with it is to
call nearest_real_pos. We can let it go if we agree to give up the
functionality it provides.
The limitation is again in your imagination, not reality. Please stop
making such foolish statements :-).
Certainly they don't justify anything you might be trying to convince
anyone of.
Please explain.
Perhaps my imagination is limited, but the only way I can think of to
simulate this functionality locally and without using any topology
information is to do an outward loop around the unreal position in
question until we find a real position. This is neither clean nor
efficient.
jason
|
|
