Ticket #699 (new enhancement)

Opened 9 years ago

Last modified 7 years ago

maphash enhancement

Reported by: mturk@… Owned by: sdlime
Priority: high Milestone:
Component: MapServer C Library Version: 4.3
Severity: minor Keywords:
Cc:

Description

Hi,

Simple patch that enhances the iterating trough hash table.
It uses the new iterator object for thread saefty.
The current implementation involves hash lookup for each table element.

Here is the sample how it is used

hashTableIteratorObj iter = msHashIteratorNew(hash_table);
while (iter->current) {
  /* iter->current is the pointer to the hashObj
   * so you can use key and data directly
   */
   printf("key = %s data = %s\n", iter->current->key, iter->current->data);

   /* advance to the next element */
   msHashIteratorNext(iter);

}

Attachments

maphash.patch.txt Download (9.3 KB) - added by mturk@… 9 years ago.
maphash enhancemt

Change History

Changed 9 years ago by mturk@…

maphash enhancemt

Changed 9 years ago by sgillies@…

  • cc sgillies@… added

Changed 9 years ago by sdlime

  • cc morissette@… added

Changed 9 years ago by sdlime

  • cc warmerdam@… added

Changed 9 years ago by fwarmerdam

I can see where this change avoids the hash lookup on each step of the 
iterator, but (in theory at least) the hash lookup should be a fast operation. 
O(log(n)). 

However, I don't see how this implementation is any more threadsafe than the
old one.  In fact, it seems more fragile in that if something has altered the
hash table (removing the current key for instance) while iterating it is easy
to get into an errant pointer situation.  The old implementation will realize
that the old pointer isn't in the hash anymore, it will gracefully continue on
to the next bucket (though potentially missing some items in the old bucket). 

I would add that introducing the whole hash iterator object adds a significant
chunk of code and complexity for no apparent purpose other than performance.  
The use of an iterator adds an easy opportunity for additional memory leaks. 

So, is the justification for the change a speed improvement?  Is hash iteration
really a performance bottleneck?  The only thing I am aware of the iterator
being used for on hashes is when saving map files.  Not generally a very
performance sensitive area. 

My advice (pending additional justification) would be to not apply this patch.

Changed 7 years ago by sgillies@…

  • cc sgillies@… removed
Note: See TracTickets for help on using tickets.