Closed Bug 1442765 Opened 2 years ago Closed 2 years ago

Switch nsTraceRefcnt's hashtables to nsClassHashtable

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
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.
(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?
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.
Andrew I think you have most expertise in this area recently, but feel free to redirect.
Attachment #8956305 - Flags: review?(continuation)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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?
Attachment #8956305 - Flags: review?(continuation) → review+
This is for use in part 2. mccr8 wanted someeone else to review it.
Attachment #8960841 - Flags: review?(nfroyd)
(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 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?
Attachment #8960841 - Flags: review?(nfroyd) → review+
erahm: both patches have r+. Can they land?
Flags: needinfo?(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.
(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.
Flags: needinfo?(erahm)
Pushed by erahm@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b60b9d07842e
https://hg.mozilla.org/mozilla-central/rev/c611225375ec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.