201034 - allow non-const enumeration, add nsTHashtable::RawRemove

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

aaronl and dwitte have a few changes to the hashtemplates they want.

1) I'm removing threadsafety from nsBaseHashtable because nobody is planning on
using it.

2) Therefore I can make nsBaseHashtable::Enumerate a non-const enumerator (and
nsCOMPtr::swap makes it possible also)

3) dwitte wants nsTHashtable::RawRemoveEntry so he can keep entrypointers
constand during an enumeration.

Comment 1

[[e-mail]]

another reason for having ::RawRemoveEntry is that it lets you avoid a second
lookup, if you've already looked up the entry you want to remove. this is a very
common situation if your entryclass has multiple children, and you only want to
remove the hash entry if all the children are gone (cookies & permissions have
this trait).

hey, a micro-optimization I know, but anything we can win... ;) it is, of
course, up to you whether you think it's worth it (i'm really not sure).

Comment 2

[Raja Harinath]

Hopefully this will also get rid of the horrendous Init(), and use the
constructor for what it was meant to.

Comment 3

[John Keiser (jkeiser)]

Init() generally exists so that you can get a return code from construction,
which you simply cannot do with a constructor.  Otherwise there is no way (short
of exceptions, which are not cross-compiler enough to support) to determine if
your object is valid or not.

Comment 4

[Benjamin Smedberg]

Attachment: reduce nsTHash codesize, add non-const enumerator, add test-suite

OK, this patch has a number of changes, for codesize and other things. I will
try to list and explain them.

1) the static initialization of nsTHashtable::sOps was eating codesize left and
right. It is now a simple data initializer, and the Init() function does the
remaining work.

2) instead of a method AllowMemMove(), an enum reduces codesize because the
compiler can optimize away an if statement.

3) fixed a major logic error in nsTHashtable::GetEntry... it didn't check
ENTRY_IS_BUSY!!! that was dumb.

4) nsTHashtable::GetEntry, PutEntry, and Clear are now inlined.

5) added RawRemove to nsTHashtable, plus a scary comment about not using it
unless necessary.

6) added non-const enumeration to nsBaseHashtable and friends

7) added a test-suite to the tests/ directory. This really helped me with
compiler errors, because they can be very difficult to understand on various
platforms. Right now it just tests normal cases, not special edge cases.

Comment 5

[Benjamin Smedberg]

Attachment: convert nsHTMLEntities.cpp to nsTHashtable (test comparison)

**** codesize numbers ****
To test codesize impact, I used this patch of nsHTMLEntities.cpp

On linux/gcc3.2: this patch costs 390 bytes (195 per hashtable).
On win32/msvc6: this patch costs 132 bytes (61 per hashtable).

Because only the functions that are used are instantiated, I think that this
will be a win when converting nsDoubleHashtable implementations, etc.  The
largest chunk of "extra" code is the Init() function (47 bytes on win32, 90 on
gcc).

Comment 6

[Benjamin Smedberg]

I can't divide. Make that 66/hashtable on win32.

Anyway, alecf voted against the threadsafety removal on nsBaseHashtable, so
unless I get really unpleasant codesize numbers testing that, it will stay.

Comment 7

[Alec Flett]

Comment on attachment 120200 [details] [diff] [review]
reduce nsTHash codesize, add non-const enumerator, add test-suite

+    NS_ASSERTION(mTable.entrySize, "nsTHashtable was not initialized
properly.");
+    
+    return (EntryType*) PL_DHashTableOperate(

can this be a NS_STATIC_CAST?

I'm not such a fan of ALLOW_MEMMOVE because it looks like a constant to me. I
mean, I guess it is supposed to be. I dunno. I think its ugly.

and what happened to it here:
-  EntryType::AllowMemMove() ? ::PL_DHashMoveEntryStub : s_CopyEntry,
+  PL_DHashMoveEntryStub,

Comment 8

[John Keiser (jkeiser)]

Comment on attachment 120200 [details] [diff] [review]
reduce nsTHash codesize, add non-const enumerator, add test-suite

Why the change to ::sOps to initialize to PL_DHashMoveEntryStub instead of
figuring out which one from AllowMemMove?  Not compiling some places?  Worried
it won't optimize out?

Looks good otherwise.  I think making ALLOW_MEMMOVE an enum is fine, since it
should never change for a given class ... but I think AllowMemMove() would be
fine too--make it inline and it will optimize out.

Comment 9

[Benjamin Smedberg]

> can this be a NS_STATIC_CAST?
done

> Why the change to ::sOps to initialize to PL_DHashMoveEntryStub instead of
> figuring out which one from AllowMemMove?  Not compiling some places?  Worried
It compiles, but it was really bad for codesize, and I thought there was a
restriction on static initializers for some port. If I initialize sOps directly,
I get a 400-byte static initializer function, plus an 8-byte guard variable; if
I just data-initialize it and do the memmove check at Init() I save both pieces.

As for the enum instead of a const static PRBool; or AllowMemMove():

1) const static PRBool doesn't optimize away properly on win32 because the
initializer must be outside the class declaration. It also means that the
ns*HashKey classes are not fully-inline.

2) static PRBool AllowMemMove() was increasing the codesize of the Init()
function by 40 bytes or so (I don't know why). I don't think it is being
optimized properly as a known-constant rval.

3) the enum looks hacky, but it works, and we use it in IDL->H translations
already. So unless you have strong objections, I'd like to keep it that way.

Comment 10

[Alec Flett]

Comment on attachment 120200 [details] [diff] [review]
reduce nsTHash codesize, add non-const enumerator, add test-suite

sr=alecf

Comment 11

[Benjamin Smedberg]

Requesting 1.4b approval. The ENTRY_IS_BUSY change is necessary for nsTHashtable
to work properly, and this patch should also fix 203030 which affects VC7.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 120200 [details] [diff] [review]
reduce nsTHash codesize, add non-const enumerator, add test-suite

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 13

[Benjamin Smedberg]

Checked in. This caused all kinds of FU on mac, because GCC3.1 apparently
doesn't believe in instantiating static template members. ccarlen and I worked
out another solution. I need to go back and get codesize numbers to make sure
that this is OK.