1438732 - Considering using nsClassHashtable for gHashTable

Status: RESOLVED WONTFIX

(Core :: Preferences: Backend, enhancement)

Description

[Eric Rahm [:erahm]]

Rather than using PLD_HashTable directly it would be cleaner to switch over to nsClassHashtable.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Use nsClassHashtable for prefs hashtable

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8951486 [details] [diff] [review]
Use nsClassHashtable for prefs hashtable

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

::: modules/libpref/Preferences.cpp
@@ +322,3 @@
>  }
>  
>  static ArenaAllocator<8192, 1> gPrefNameArena;

Please put the blank line back.

@@ +695,5 @@
> +
> +// Hashtable used to map names to pref objects. StringArenaHashKey is used to
> +// allocate the strings out of an arena.
> +using PrefTable = nsClassHashtable<StringArenaHashKey, Pref>;
> +static PrefTable* gHashTable;

I'd prefer 'PrefsTable' to 'PrefTable'.

@@ +788,1 @@
>  pref_HashTableLookupInner(const char* aPrefName)

Is this function still needed? Can it be merged with pref_HashTableLookup()?

@@ +812,5 @@
>  
>  static Pref*
>  pref_HashTableLookup(const char* aPrefName)
>  {
> +  auto entry = pref_HashTableLookupInner(aPrefName);

Please use the type name here instead of 'auto'.

@@ +831,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  // Technically going from fallible to infallible. We'd need to add fallible
> +  // version of LookupForAdd if we still want fallibility.

I think infallible is fine here, and this comment can be removed.

@@ +832,5 @@
>    }
>  
> +  // Technically going from fallible to infallible. We'd need to add fallible
> +  // version of LookupForAdd if we still want fallibility.
> +  auto pref = gHashTable->LookupOrAdd(aPrefName);

Please use the type name here instead of 'auto'.

@@ +3043,5 @@
>  {
>    ENSURE_PARENT_PROCESS("Preferences::ResetPrefs", "all prefs");
>  
> +  // TODO(erahm): Maybe add version of clear that takes a length. This function
> +  // is only used in tests so maybe it doesn't matter.

Yeah, probably doesn't matter.

@@ +4083,4 @@
>  {
>    NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
>  
> +  auto entry = pref_HashTableLookupInner(aPrefName);

Please use the type name instead of 'auto' here.

Comment 3

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Comment on attachment 8951486 [details] [diff] [review]
> Use nsClassHashtable for prefs hashtable
> 
> Review of attachment 8951486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpref/Preferences.cpp
> @@ +322,3 @@
> >  }
> >  
> >  static ArenaAllocator<8192, 1> gPrefNameArena;
> 
> Please put the blank line back.

Sorry, copy and paste went awry. Will add back.

> @@ +695,5 @@
> > +
> > +// Hashtable used to map names to pref objects. StringArenaHashKey is used to
> > +// allocate the strings out of an arena.
> > +using PrefTable = nsClassHashtable<StringArenaHashKey, Pref>;
> > +static PrefTable* gHashTable;
> 
> I'd prefer 'PrefsTable' to 'PrefTable'.

WFM.

> @@ +788,1 @@
> >  pref_HashTableLookupInner(const char* aPrefName)
> 
> Is this function still needed? Can it be merged with pref_HashTableLookup()?

It's used in one other place, but seems unnecessary. Will merge w/ the wrapper.

> @@ +812,5 @@
> >  
> >  static Pref*
> >  pref_HashTableLookup(const char* aPrefName)
> >  {
> > +  auto entry = pref_HashTableLookupInner(aPrefName);
> 
> Please use the type name here instead of 'auto'.

Sure.

> 
> @@ +831,5 @@
> >      return NS_ERROR_OUT_OF_MEMORY;
> >    }
> >  
> > +  // Technically going from fallible to infallible. We'd need to add fallible
> > +  // version of LookupForAdd if we still want fallibility.
> 
> I think infallible is fine here, and this comment can be removed.

Okay, will update.

> @@ +832,5 @@
> >    }
> >  
> > +  // Technically going from fallible to infallible. We'd need to add fallible
> > +  // version of LookupForAdd if we still want fallibility.
> > +  auto pref = gHashTable->LookupOrAdd(aPrefName);
> 
> Please use the type name here instead of 'auto'.

Okay.

> @@ +3043,5 @@
> >  {
> >    ENSURE_PARENT_PROCESS("Preferences::ResetPrefs", "all prefs");
> >  
> > +  // TODO(erahm): Maybe add version of clear that takes a length. This function
> > +  // is only used in tests so maybe it doesn't matter.
> 
> Yeah, probably doesn't matter.

Will remove the TODO.

> @@ +4083,4 @@
> >  {
> >    NS_ENSURE_TRUE(InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
> >  
> > +  auto entry = pref_HashTableLookupInner(aPrefName);
> 
> Please use the type name instead of 'auto' here.

Will do (also switching just to using pref_HashTableLookup).

Comment 4

[Eric Rahm [:erahm]]

Oh right, it turns out `pref_HashTableLookupInner` is used to optimize the removal in `ClearUserInAnyProcess`.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/633b2c102c95
Use nsClassHashtable for prefs hashtable. r=njn

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/633b2c102c95

Comment 7

[Nicholas Nethercote [inactive]]

I was just looking at about:memory and I see that this changed has increased memory usage of prefs by 64 KiB per process on 64-bit and 32 KiB per process on 32-bit.

On 64-bit:

- sizeof(PrefEntry) increases from 16 to 24 bytes, because it now has the name pointer. The prefs table has 8192 entries, so that's a 64 KiB increase, from 128 KiB to 192 KiB.

- sizeof(Pref) reduces from 32 bytes to 24 bytes, but each Pref is heap-allocated, and mozjemalloc rounds up the 24 to 32, so there is no change there.

On 32-bit:

- sizeof(PrefEntry) increase from 8 to 12, so that's a 32 KiB increase in the table size.

- sizeof(Pref) reduces from 16 to 12, but mozjemalloc rounds that up to 16, again giving no change there.

This will also reduce the effectiveness of statically allocating default Pref objects (bug 1437168) because they are smaller.

I don't think the improvement in code readability is sufficient to suffer this per-process overhead increase, and therefore it should be backed out. Sorry that I didn't catch this at review time.

erahm, what do you think?

Comment 8

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> erahm, what do you think?

Okay, I'll r+ if you want to backout.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c26afcddaea20455d1c9f546a3072543e3b9d73
Back out 633b2c102c95 (bug 1438732) because it increases memory usage. r=erahm