200709 - nsTHashtable cleanup

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

timeless pointed out a couple of warnings that I introduces with nsTHashtable,
and they are valid. So I'm fixing that right now.

Also, as I mentioned in bug 180264, the dynamic linking of the hashtable
templates isn't worth it. So I'm going to remove that code. I can also integrate
the nsHash*Impl files, because with implicit linking the implementation doesn't
need to be separate.

Comment 1

[Benjamin Smedberg]

Attachment: fix warnings in nsHashKeys.h

We have to take the address of the hash key, so PRUint32 needs to be a
reference. And we need a public inheritance to do the cast properly in the
enumerator.

Comment 2

[Alec Flett]

Comment on attachment 119479 [details] [diff] [review]
fix warnings in nsHashKeys.h

sr=alecf

Comment 3

[Aaron Leventhal]

Benjamin, do you have any examples of using EnumerateEntries?

Comment 4

[John Keiser (jkeiser)]

Fix checked in.

Comment 5

[John Keiser (jkeiser)]

Benjamin, I'll leave this open for you if you want to do the dynamic linking
changes here too.

Comment 6

[Benjamin Smedberg]

Attachment: part two: remove dynamic linking of hash templates

This is mainly a cut-n-paste job. It moves the *Impl.h files into the matching
main file. It also removes the leftover #define cruft from the dynamic linking.


I also added PL_EXTERN/PR_IMPLEMENT to PL_DHashStubEnumRemove because it will
be dynamically linked.

After the patch is checked in ds/nsTHashtableImpl.h ds/nsBaseHashtableImpl.h
and ds/nsInterfaceHashtableImpl.h all need to be 'cvs remove'd.

Comment 7

[Alec Flett]

I'm sorry, what's the motivation here, for removing the dynamic linking?
dont' we WANT to share code?

Comment 8

[Alec Flett]

ah, I just read a bit of history.
I think we want this stuff to be a part of the xpcom dll. I realize theres a
codesize increase, but that code should be (I would hope) shared among dlls.

Have we done any tests (or added any nsTHashtable tests to xpcom/tests) to
determine the codesavings over using a raw pldhashtable? I have to imagine there
is some win there?

Try converting nsStaticNameTable or something, see what kind of improvement you
get. (or even the implementation behind nsHashtable, which currently uses pldhash)

Comment 9

[Benjamin Smedberg]

> I think we want this stuff to be a part of the xpcom dll. I realize theres a
> codesize increase, but that code should be (I would hope) shared among dlls.

On win32, the cost of dynamic linking vastly outweighs the actual code cost:
4.5k of linking definitions in the xpcom dll *and in the importing dll* for 1k
of code.

> determine the codesavings over using a raw pldhashtable? I have to imagine there
> is some win there?

Because nsTHashtable and friends are typesafe wrapper classes, there isn't
likely to be any win using nsTHashtable instread of coding using pldhash
directly; so people ought not be discouraged from using pldhash directly if they
are up to the challenge. There probably isn't any loss either... that the two
methods ought to be roughly equivalent: most of the codesize cost is the
pldhashops function definitions which you have either way. I'm doing a couple
conversions to test this belief factually using codesighs.

--BDS

As a side note to myself: inlining nsTHashtable::PutEntry and GetEntry is
probably a win.

Removing the threadsafety code from nsBaseHashtable may also allow me to inline
a few more fuctions there. It seems that my private project is the only one that
has any use for a threadsafe hashtable, so it's probably better to just axe it.

Comment 10

[Alec Flett]

nsHashtable supports threadsafety, so I guess if there isn't a HUGE overhead,
I'd like to continue supporting it.

I am disappointed to hear that there isn't a greater win here, but I'd like to
see some results from codesighs. I mean, if we save even 256 bytes per
implementation, that would be huge...but if its more like 16 bytes, then I
suppose that's not much of a win.

Comment 11

[John Keiser (jkeiser)]

Comment on attachment 119571 [details] [diff] [review]
part two: remove dynamic linking of hash templates

I think the lesson here is, "don't try to share small classes across DLLs."  Or
maybe just templatized classes.  At least on windows the dll import linkage is
likely to be > 1K, which means you may as well just have your own copy of the
code.

Even if there were *zero* import bloat, at 1K for the class and 4.5K for
linkage information, you have to use the class in 6 DLLs before dynamic linking
becomes a win.	And since there is going to be some import bloat (maybe a lot),
that number is already conservative.

Oh, and there *is* a reason to encourage even experienced PLDHashTable users to
use this class instead of that.  The experienced PLDHashTable users aren't the
only ones who are going to modify the code in the future.  We should code for
those people rather than the experienced hashers if we can.

Comment 12

[Alec Flett]

Comment on attachment 119571 [details] [diff] [review]
part two: remove dynamic linking of hash templates

ok, if you say so :)
sr=alecf

Comment 13

[Benjamin Smedberg]

Fixed, many apologies for the bustage. I've gotten some interesting codesize
numbers and supplemental fixes that I'm putting in bug 201034.