WITH_HASH_MAP

[Ralf Corsepius]

On Wed, 2006-06-28 at 06:25 -0700, Panu Matilainen wrote:
> On Wed, 28 Jun 2006, Ralf Corsepius wrote:
> 
> > Hi,
> >
> > apt-pkg/rpm/* contains a conditionally compiled code using a define
> > named WITH_HASH_MAP.
> >
> > AFAIS, this seems to be an incomplete attempt to use hash_map<> instead
> > of map<> for some kind of dictionary operations.
> >
> > ATM, WITH_HASH_MAP is never defined, so the code will never be compiled
> > nor used.
> 
> Did a bit of digging into the cnc svn repository... it was originally 
> added here as a configure option --with-hashmap:
> svn diff -r77:79 https://oops.kerneljanitors.org/repos/apt-rpm/trunk
> 
> It's then later (-r116:117 from the repo above) converted to be 
> autodetected and always used if ext/hashmap is present... or supposed to: 
> the define changes from WITH_HASH_MAP to WITH_GNU_HASH_MAP but the change 
> is not done (by mistake I believe) to all the places, making it 
> non-functional.
Hmm, ... Oversight or symptom of somebody "poking at bugs"?

> > Question: What to do?
> >
> > If we want to get this functional, we should at least add some magic
> > corresponding magic to the configure script to activate this code.
> > If we don't want to get this functional, I'd be in favor of eliminating
> > this "dead code".
> >
> > Subquestions, I currently can't answer:
> >
> > * Would be using hash_map<>'s be worth it?
> > Generally speaking, my attitude on them is ambivalent: On one hand, they
> > are non-portable, tedious to use and an additional source of errors, but
> > promise a speed up. Is such a speed up sensible/measurable in these
> > particular cases? I don't know.
> >
> > * Would be keeping the current implementation of hash_map<> support be
> > worth it?
> > IMO, the current implementation is problematic (to say the least).
> > I do not think keeping it is worth it.
> >
> > For the moment, I'd propose to add another option to configure (say
> > --enable-experimental), which would optionally compile apt with
> > "WITH_HASH_MAP". We'd then pretty soon know if this code is functional
> > at all, and if it provides a measurable speed up.
> 
> All that's needed is s/WITH_HASH_MAP/WITH_GNU_HASH_MAP/ in the rpm 
> directory to resurrect the thing that's supposed to have been enabled all 
> these years (been unused since March 2003).
This is only half of the story, ...

* these defines pollute the API: They are being used in exported
headers. A true "must-be-fixed" design flaw (I am inclined to think none
of the rpm*.h should be exported, but haven't tried to analyze the
details).

* GCC's hash_maps have a very bumpy history. For example, their
namespaces are *documented* to have changed several times, which renders
supporting older GCCs tedious. The current check in configure.ac ignores
these incompatibilities and tries be restrictive. 

> So re-enabling it is trivial enough, profiling whether it actually buys us 
> anything is another thing. The comments in svn say "provides good speed 
> improvement" but... I'll try to have a look at it tomorrow along with 
> various other things.
I've tried to benchmark lookups of map<string,int> vs
hash_map<string,int>. 

These benchmarks indicate string hash_maps to be ca. 30% faster than
string maps. However, I am not sure if it buys anything to apt, because
at least for me, lookups don't seem to be the bottleneck of apt's
performance.

Ralf