1442765 - Switch nsTraceRefcnt's hashtables to nsClassHashtable

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Eric Rahm [:erahm]]

e should really avoid using PL_Hash if possible. Converting `gBloatView`, `gTypesToLog`, `gObjectsToLog`, and `gSerialNumbers`[1] is pretty straightforward.

While we're messing with them we could probably store them in nsStaticPtr's as well.

[1] https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/xpcom/base/nsTraceRefcnt.cpp#81-84

Comment 1

[Andrew McCreight [:mccr8]]

These hashtables are like this because we don't want to implement leak checking using tables that leak checking tracks. There's a comment in there about it somewhere. That said, I haven't thought through the precise limitations of it.

Comment 2

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> These hashtables are like this because we don't want to implement leak
> checking using tables that leak checking tracks. There's a comment in there
> about it somewhere. That said, I haven't thought through the precise
> limitations of it.

Not seeing the comment...does leak checking really track these tables or would it just potentially track things in the tables? Can you point to where the tracking is done so I can familiarize myself with it?

Comment 3

[Andrew McCreight [:mccr8]]

Hmm, ok, it looks like we don't have leak checking for them (which seems bad, but anyways). The comment I saw was actually talking about the use of std::vector<>, not hash tables. Anyways, if it works with a debug build I guess it is okay.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Switch nsTraceRefcnt's hashtables to use xpcom hashtables

Andrew I think you have most expertise in this area recently, but feel free to redirect.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8956305 [details] [diff] [review]
Switch nsTraceRefcnt's hashtables to use xpcom hashtables

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

Please be sure to test this. Just make sure that you can do something like XPCOM_MEM_BLOAT_LOG=./rc.log XPCOM_MEM_LOG_CLASSES=nsTimer ./mach run and the rc.log files have stuff in them. Also throw something like XPCOM_MEM_LOG_OBJECTS=5-20 in there.

::: xpcom/base/nsTraceRefcnt.cpp
@@ +78,4 @@
>    ~AutoTraceLogLock() { if (doRelease) gTraceLogLocked = 0; }
>  };
>  
> +class IntPtrHashKey : public PLDHashEntryHdr

Please move this to nsHashKeys and get an XPCOM person to review it. I'm not confident in my ability to review a new hash key.

@@ +102,5 @@
> +class BloatEntry;
> +struct SerialNumberRecord;
> +
> +using BloatHash = nsClassHashtable<nsDepCharHashKey, BloatEntry>;
> +using CharSet = nsTHashtable<nsCharPtrHashKey>;

Shouldn't this be StringSet? It is a set of char*, not char.

@@ +384,5 @@
>    }
> +  BloatEntry* entry = gBloatView->Get(aTypeName);
> +  if (!entry && aInstanceSize > 0) {
> +    entry = new BloatEntry(aTypeName, aInstanceSize);
> +    if (!gBloatView->Put(aTypeName, entry, mozilla::fallible)) {

Does this need to be fallible? Also, it feels like you could do something with UniquePtr and Move in here to avoid the delete entry, but I suppose that would complicate things a bit...

@@ +536,2 @@
>  static bool
>  LogThisType(const char* aTypeName)

Maybe just inline this method?

@@ +565,2 @@
>  {
> +  auto record = gSerialNumbers->Get(aPtr);

I'd just inline this given that it is only called in one place. Also, the return value is not used, so get rid of that at least.

@@ +571,5 @@
> +  return -1;
> +}
> +
> +static int32_t
> +DecrRefCount(void* aPtr)

Likewise.

@@ +583,5 @@
>  }
>  
>  #ifdef HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR
> +static int32_t
> +IncrCOMPtrCount(void* aPtr)

Likewise. Though this at least uses the return value.

@@ +594,5 @@
> +  return -1;
> +}
> +
> +static int32_t
> +DecrCOMPtrCount(void* aPtr)

Likewise.

@@ +1242,4 @@
>  {
>    gCodeAddressService = nullptr;
>    if (gBloatView) {
> +    delete gBloatView;

Maybe make all of these StaticAutoPtr?

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 1: Add uintptr_t hashkey type

This is for use in part 2. mccr8 wanted someeone else to review it.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8956305 [details] [diff] [review]
> Switch nsTraceRefcnt's hashtables to use xpcom hashtables
> 
> Review of attachment 8956305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please be sure to test this. Just make sure that you can do something like
> XPCOM_MEM_BLOAT_LOG=./rc.log XPCOM_MEM_LOG_CLASSES=nsTimer ./mach run and
> the rc.log files have stuff in them. Also throw something like
> XPCOM_MEM_LOG_OBJECTS=5-20 in there.
> 
> ::: xpcom/base/nsTraceRefcnt.cpp
> @@ +78,4 @@
> >    ~AutoTraceLogLock() { if (doRelease) gTraceLogLocked = 0; }
> >  };
> >  
> > +class IntPtrHashKey : public PLDHashEntryHdr
> 
> Please move this to nsHashKeys and get an XPCOM person to review it. I'm not
> confident in my ability to review a new hash key.

Added a part 1 with this.

> @@ +102,5 @@
> > +class BloatEntry;
> > +struct SerialNumberRecord;
> > +
> > +using BloatHash = nsClassHashtable<nsDepCharHashKey, BloatEntry>;
> > +using CharSet = nsTHashtable<nsCharPtrHashKey>;
> 
> Shouldn't this be StringSet? It is a set of char*, not char.

In our hashkey nomenclature I guess it should be CharPtrSet, StringSet would imply nsString.

> @@ +384,5 @@
> >    }
> > +  BloatEntry* entry = gBloatView->Get(aTypeName);
> > +  if (!entry && aInstanceSize > 0) {
> > +    entry = new BloatEntry(aTypeName, aInstanceSize);
> > +    if (!gBloatView->Put(aTypeName, entry, mozilla::fallible)) {
> 
> Does this need to be fallible? Also, it feels like you could do something
> with UniquePtr and Move in here to avoid the delete entry, but I suppose
> that would complicate things a bit...

I was just preserving previous behavior, I can make it infallible which would get rid of the free on failure logic too.

> @@ +536,2 @@
> >  static bool
> >  LogThisType(const char* aTypeName)
> 
> Maybe just inline this method?
> 
> @@ +565,2 @@
> >  {
> > +  auto record = gSerialNumbers->Get(aPtr);
> 
> I'd just inline this given that it is only called in one place. Also, the
> return value is not used, so get rid of that at least.
> 
> @@ +571,5 @@
> > +  return -1;
> > +}
> > +
> > +static int32_t
> > +DecrRefCount(void* aPtr)
> 
> Likewise.
> 
> @@ +583,5 @@
> >  }
> >  
> >  #ifdef HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR
> > +static int32_t
> > +IncrCOMPtrCount(void* aPtr)
> 
> Likewise. Though this at least uses the return value.
> 
> @@ +594,5 @@
> > +  return -1;
> > +}
> > +
> > +static int32_t
> > +DecrCOMPtrCount(void* aPtr)
> 
> Likewise.

I'll inline all of them.
 
> @@ +1242,4 @@
> >  {
> >    gCodeAddressService = nullptr;
> >    if (gBloatView) {
> > +    delete gBloatView;
> 
> Maybe make all of these StaticAutoPtr?

Will do.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8960841 [details] [diff] [review]
Part 1: Add uintptr_t hashkey type

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

WFM.  Your commit message says uintptr_t, but the patch says intptr_t.  Which one is it?

::: xpcom/ds/nsHashKeys.h
@@ +286,4 @@
>  };
>  
>  /**
> + * hashkey wrapper using uintptr_t KeyType

intptr_t key, surely?

Comment 9

[Nicholas Nethercote [inactive]]

erahm: both patches have r+. Can they land?

Comment 10

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> erahm: both patches have r+. Can they land?

Thanks for the ping! It looks like both had changes requested, I'll see if I can dig up the updated patches.

Comment 11

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8956305 [details] [diff] [review]
> Switch nsTraceRefcnt's hashtables to use xpcom hashtables
> 
> Review of attachment 8956305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please be sure to test this. Just make sure that you can do something like
> XPCOM_MEM_BLOAT_LOG=./rc.log XPCOM_MEM_LOG_CLASSES=nsTimer ./mach run and
> the rc.log files have stuff in them. Also throw something like
> XPCOM_MEM_LOG_OBJECTS=5-20 in there.

> MOZ_DISABLE_CONTENT_SANDBOX=1 XPCOM_MEM_BLOAT_LOG=./rc.log \
> XPCOM_MEM_LOG_CLASSES=nsTimer XPCOM_MEM_LOG_OBJECTS=5-20 \
> ./mach run

Appears to function.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60b9d07842e
Part 1: Add intptr_t hashkey type. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c611225375ec
Part 2: Switch nsTraceRefcnt's hashtables to use xpcom hashtables. r=mccr8

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/b60b9d07842e
https://hg.mozilla.org/mozilla-central/rev/c611225375ec