nsTHashtable cleanup

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
XPCOM
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.4beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 119479 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #119479 - Flags: superreview?(alecf)
Attachment #119479 - Flags: review?(jkeiser)

Comment 2

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

sr=alecf
Attachment #119479 - Flags: superreview?(alecf) → superreview+

Updated

15 years ago
Attachment #119479 - Flags: review?(jkeiser) → review+

Comment 3

15 years ago
Benjamin, do you have any examples of using EnumerateEntries?

Comment 4

15 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 5

15 years ago
Benjamin, I'll leave this open for you if you want to do the dynamic linking
changes here too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 6

15 years ago
Created attachment 119571 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #119571 - Flags: superreview?(alecf)
Attachment #119571 - Flags: review?(jkeiser)

Comment 7

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

Comment 8

15 years ago
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)
(Assignee)

Comment 9

15 years ago
> 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.
(Assignee)

Updated

15 years ago
Blocks: 201034

Comment 10

15 years ago
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 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.
Attachment #119571 - Flags: review?(jkeiser) → review+

Comment 12

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

ok, if you say so :)
sr=alecf
Attachment #119571 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 13

15 years ago
Fixed, many apologies for the bustage. I've gotten some interesting codesize
numbers and supplemental fixes that I'm putting in bug 201034.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.4beta
You need to log in before you can comment on or make changes to this bug.