[Freeciv-Dev] Re: improper access of iterators

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 26, 2001 at 05:40:42PM -0400, Justin Moore wrote:
> 
> On Sun, 26 Aug 2001, Raimar Falke wrote:
> > On Sun, Aug 26, 2001 at 03:29:01PM -0400, Justin Moore wrote:
> > >
> > >    In my quest to improve genlist efficiency I've stumbled across some
> > > improper use of variables within the AI and server code.  Look at
> > > ai/aicity.c:1036.  The unit_list_iterate is defined in terms of the
> > > TYPED_LIST_ITERATE macro in common/genlist.h.  The current macro uses a
> > > genlist_iterator named myiter that the AI and server code references
> > > directly, even though the iterator isn't specifically initialized or even
> > > named within the calling code.
> > >
> > >    What are people's thoughts about me migrating from the old genlist to
> > > my new genlist?  My current version of genlist is backwards compatible
> > > with the old one.  The implementation of the ITERATOR_* macros are kind of
> > > hacks and are probably slightly less efficient than the old ones, but I
> > > plan to phase those out if possible.  My idea is to migrate code from the
> > > older, slower genlist data structures and iterators to the newer, faster
> > > ones.  It would be in a few steps.  If people would like more info on
> > > this, I'd be happy to provide it.  I really think we could get a good
> > > performance gain out of a more efficient genlist, but I'd like some
> > > feedback on my ideas.
> >
> > You have to show code or provide more details. Overall I think
> > dynamically resized arrays are faster then double linked lists. I
> > don't think there will be a lot of overall performance gain. I would
> > like to see an interface genlist.h and implementations like
> > array_genlist.h and dll_genlist.h. This would allow sufficient
> > performance testing.
> 
>    I'm working on providing some profiling results.  Part of the point of
> my post was that it's not quite as simple as re-linking against an array
> implementation; some programmers assumed the existance of an iterator
> named 'myiter' in a few parts of the code.  I'd have to clean out lots of
> those places for a good comparison, resulting in some sizeable patches.

How do you change these things? I would change things like
  unit_list_iterate(pcity->units_supported, punit)
    if (some_condition) {
       handle_unit_disband_safe(pplayer, &pack, &myiter);
       city_refresh(pcity);
       ai_fix_unhappy(pcity);
     }
  unit_list_iterate_end;

to
for(;;) {
  int changed=0;
  unit_list_iterate(pcity->units_supported, punit)
    if (some_condition) {
       handle_unit_disband_safe(pplayer, &pack);
       city_refresh(pcity);
       ai_fix_unhappy(pcity);
       changed=1;
       break;
     }
  unit_list_iterate_end;
  if(changed) break;
}

This looks save to me but is a bit ugly.

> I guess I'm just asking if it would be worth my time to do this.  I
> feel that between the new genlist and some key sortlist
> improvements, we could improve speed by 2-3% (IMHO), but again:
> would the maintainers be interested in these speedups if available?

IMHO it depends on the speedup and how you get rid of the myiter (how
nice it would get).

        Raiamr

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "- Amiga Y2K fixes (a bit late, wouldn't you say?)"
    -- Linus Torvalds about linux 2.4.0 at 4 Jan 2001