Closed Bug 1438732 Opened 4 years ago Closed 3 years ago

Considering using nsClassHashtable for gHashTable

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(1 file)

Rather than using PLD_HashTable directly it would be cleaner to switch over to nsClassHashtable.
Attachment #8951486 - Flags: feedback?(n.nethercote)
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.
Attachment #8951486 - Flags: feedback?(n.nethercote) → review+
(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).
Oh right, it turns out `pref_HashTableLookupInner` is used to optimize the removal in `ClearUserInAnyProcess`.
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/633b2c102c95
Use nsClassHashtable for prefs hashtable. r=njn
https://hg.mozilla.org/mozilla-central/rev/633b2c102c95
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(erahm)
Resolution: FIXED → ---
(In reply to Nicholas Nethercote [:njn] from comment #7)
> erahm, what do you think?

Okay, I'll r+ if you want to backout.
Flags: needinfo?(erahm)
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.