Closed Bug 1452288 Opened 3 years ago Closed 3 years ago

PLDHashTable should use calloc

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file)

We should switch over the entry array allocation in PLDHashTable to calloc rather than malloc+memset as it might be more efficient.
Duplicate of this bug: 1435283
Comment on attachment 8965872 [details] [diff] [review]
Use calloc for allocating PLDHashTable entries

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

Thank you!  r=me with the changes below.

::: xpcom/ds/PLDHashTable.cpp
@@ +476,4 @@
>      return false;   // overflowed
>    }
>  
> +  char* newEntryStore = (char*)calloc(nbytes);

Wait, calloc takes *two* arguments, yes?  So this should really be `calloc(1, nbytes)` or somesuch?

@@ +554,4 @@
>      // We already checked this in the constructor, so it must still be true.
>      MOZ_RELEASE_ASSERT(SizeOfEntryStore(CapacityFromHashShift(), mEntrySize,
>                                          &nbytes));
> +    mEntryStore.Set((char*)calloc(nbytes), &mGeneration);

Same story here.
Attachment #8965872 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8965872 [details] [diff] [review]
> Use calloc for allocating PLDHashTable entries
> 
> Review of attachment 8965872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you!  r=me with the changes below.
> 
> ::: xpcom/ds/PLDHashTable.cpp
> @@ +476,4 @@
> >      return false;   // overflowed
> >    }
> >  
> > +  char* newEntryStore = (char*)calloc(nbytes);
> 
> Wait, calloc takes *two* arguments, yes?  So this should really be
> `calloc(1, nbytes)` or somesuch?
> 
> @@ +554,4 @@
> >      // We already checked this in the constructor, so it must still be true.
> >      MOZ_RELEASE_ASSERT(SizeOfEntryStore(CapacityFromHashShift(), mEntrySize,
> >                                          &nbytes));
> > +    mEntryStore.Set((char*)calloc(nbytes), &mGeneration);
> 
> Same story here.

Sorry, yes that was updated locally :)
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd93e695965
Use calloc for allocating PLDHashTable entries. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/4cd93e695965
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.