WITH_HASH_MAP

[Panu Matilainen]

On Wed, 2006-06-28 at 16:18 +0200, Ralf Corsepius wrote:
> 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"?

I would think it's just an oversight by Gustavo (nobody else at the time
had commit access to the repository :)

> 
> > > 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).

Indeed, all of the rpm/*.h stuff should really be private. Why it's done
the way it is now, I dunno...

> 
> * 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.

Yup, from what little I've profiled apt, such optimizations are more
likely to just get lost in the noise.

	- Panu -