1247850 - Shrink NameTableKey in nsStaticCaseInsensitiveNameTable

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

nsStaticCaseInsensitiveNameTable is basically a lookup table for strings. It
can also map these strings to enum values.

We start with an array of C strings (const char*), create another array of
nsDependentCStrings (mNameArray) that all point to the first array, and then 
create a hash table (mNameTable) to enable fast lookup of those 
nsDependentCStrings.

Each entry in the hash table (NameTableEntry) has a pointer to the
nsDependentCString and also its index in the array. But we don't need both --
you can get the nsDependentCString address from the index (plus the array
pointer). So we can remove the pointer, which reduces sizeof(NameTableEntry)
from 24 bytes (on 64-bit) or 16 bytes (on 32-bit) to 8 bytes.

We have a few of these tables and this ends up saving 41.5 KiB per process on
64-bit, and a bit less on 32-bit (I haven't measured the saving on 32-bit).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Shrink NameTableKey in nsStaticCaseInsensitiveNameTable

This patch removes NameTableEntry::mString. This requires adding mNameArray to
NameTableKey so that we can index off it in matchNameKeysCaseInsensitive().

This change saves 41.5 KiB per process.

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8718716 [details] [diff] [review]
Shrink NameTableKey in nsStaticCaseInsensitiveNameTable

Review of attachment 8718716 [details] [diff] [review]:
-----------------------------------------------------------------

I'd feel much more comfortable with this if we added a test for |nsStaticCaseInsensitiveNameTable|. Visually it seems ok, I assume the reduction in size was measured on 64-bit?

::: xpcom/ds/nsStaticNameTable.cpp
@@ +139,5 @@
>      if (!entry) {
>        continue;
>      }
>  
> +    NS_ASSERTION(entry->mIndex == 0, "Entry already exists!");

This assertion doesn't really make sense anymore, an entry of 0 could already be present.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to Eric Rahm [:erahm] from comment #2)
> I assume the
> reduction in size was measured on 64-bit?

Yes, as per comment 0. The saving is 41.5 KiB on 64-bit and will be roughly half that on 32-bit (the exact number depends on how jemalloc ends up rounding up the requests).

 
> > +    NS_ASSERTION(entry->mIndex == 0, "Entry already exists!");
> 
> This assertion doesn't really make sense anymore, an entry of 0 could
> already be present.

Good catch, and thumbs down for non-fatal assertions.

Comment 4

[Nicholas Nethercote [inactive]]

> > > +    NS_ASSERTION(entry->mIndex == 0, "Entry already exists!");
> > 
> > This assertion doesn't really make sense anymore, an entry of 0 could
> > already be present.
> 
> Good catch, and thumbs down for non-fatal assertions.

Actually, it does still make sense. If the entry is already in use (i.e. we have a bug):

- if the index is non-zero (the common case) the assertion will trigger. Good.

- if the index is zero (the uncommon case) the assertion won't trigger. A shame.

So it gives us not quite 100% coverage, which is still much better than 0%.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e55086409f41d720799814e4ce3b6f274cfcb1b0
Bug 1247850 - Shrink NameTableKey in nsStaticCaseInsensitiveNameTable. r=froydnj,erahm.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e55086409f41