1004609 - Use DMD's LocationService for nsTraceRefcnt's refcount logging

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Refcount logging is slow.  NS_DescribeCodeAddress takes most of the time.  DMD has already circumvented this problem by writing a cache for the results of NS_DescribeCodeAddress called LocationService.  We should use this for nsTraceRefcnt::WalkTheStack.

The main problem is going to be making code that deals with the particular requirements of DMD while also working in the browser, though it looks like there are only a few integration points.

I hacked up a prototype for this, and it makes refcount logging fast enough that you can refcount log nsDocShell and the browser windows comes up after 15 seconds, which is amazingly fast.  The cache has about a 99% hit rate.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Make nsTraceRefcnt::WalkTheStack use DMD's LocationService. WIP.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Make nsTraceRefcnt::WalkTheStack use DMD's LocationService. WIP.

Oops forget to include the new file.

Comment 3

[Andrew McCreight [:mccr8]]

So, Nathan, which do you think is the lesser of two evils for dealing with the hashtable here: I could just use the JS hash table or I could templatize the hashtable implementation.  I only need a few operations so it wouldn't be too terrible.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: WIP, folded together

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> So, Nathan, which do you think is the lesser of two evils for dealing with
> the hashtable here: I could just use the JS hash table or I could templatize
> the hashtable implementation.  I only need a few operations so it wouldn't
> be too terrible.

Another option: mfbt/Vector.h was originally js/public/Vector.h. You could likewise move js/public/Hashtable.h into MFBT.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> So, Nathan, which do you think is the lesser of two evils for dealing with
> the hashtable here: I could just use the JS hash table or I could templatize
> the hashtable implementation.  I only need a few operations so it wouldn't
> be too terrible.

I think between these two, I like templatizing LocationService better.  I'm -0 on moving JS's template-heavy hashtable into MFBT.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Inline the W() macro in LocationService.

This will make it easier to move it later, and is barely used in the LocationService.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Templatize LocationService over the writer.

try run: https://tbpl.mozilla.org/?tree=Try&rev=4ccdf4dd033a

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Templatize LocationService over the string interner.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Templatize LocationService over the string allocator.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Templatize LocationService over the lock.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Add empty LocationService.h file.

I just split this out to make rebasing over changes to the location service easier.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Move LocationService into xpcom.

I asked both of you for review because this kind of involves two things: is it okay to move this out of dmd/ and is it okay to move into xpcom. One but not both of you should feel free to not actually review it.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Implement a caching version of WalkTheStack using LocationService.

Then use it in ref counting.

This only uses the cached version for refcounting, because the location service uses a lot of memory, so it is overkill if you just want to call it once for NS_ASSERTION.  The acute reason for not doing it is that in fact Mochitests blow up on TBPL when it tries to allocate the location service.

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8464191 [details] [diff] [review]
part 2 - Templatize LocationService over the writer.

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

::: memory/replace/dmd/DMD.cpp
@@ +772,5 @@
>    size_t NumCacheHits()   const { return mNumCacheHits; }
>    size_t NumCacheMisses() const { return mNumCacheMisses; }
>  };
>  
> +typedef LocationService<Writer> DMDLocationService;

Could this be dmd::LocationService instead? I've generally avoided "DMD" prefixes in the DMD code in favour of the |dmd| namespace.

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8464192 [details] [diff] [review]
part 3 - Templatize LocationService over the string interner.

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

::: memory/replace/dmd/DMD.cpp
@@ +559,5 @@
>  //---------------------------------------------------------------------------
>  
>  // This class is used to print details about code locations.
>  //
> +// StringTable must implement an Intern() method that returns an interned

nit: "|StringTable|".

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8464193 [details] [diff] [review]
part 4 - Templatize LocationService over the string allocator.

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

::: memory/replace/dmd/DMD.cpp
@@ +563,5 @@
>  // StringTable must implement an Intern() method that returns an interned
>  // copy of the string that was passed in, as well as a standard
>  // SizeOfExcludingThis() method.
>  //
> +// StringAlloc must implement |copy| and |free|. |copy| copies a string,

nit: "|StringAlloc|".

@@ +803,5 @@
>  
>    StringHashSet mSet;
>  };
>  
> +struct StringAlloc

Can you make this a class instead of a struct? Just for consistency with other top-level classes.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8464194 [details] [diff] [review]
part 5 - Templatize LocationService over the lock.

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

::: memory/replace/dmd/DMD.cpp
@@ +568,5 @@
>  // while |free| is used to free strings created by |copy|.
>  //
>  // Writer must implement a printf-style varargs method Write.
> +//
> +// UnlockState is needed when the callers may be holding a lock

nit: "|UnlockState|". Do I sound like a broken record yet? :)

@@ +822,5 @@
> +// NS_DescribeCodeAddress can (on Linux) acquire a lock inside
> +// the shared library loader.  Another thread might call malloc
> +// while holding that lock (when loading a shared library).  So
> +// we have to exit gStateLock around this call.  For details, see
> +// https://bugzilla.mozilla.org/show_bug.cgi?id=363334#c3

You've moved this comment and it now doesn't make sense, i.e. what is "this call"?

I think it should be moved back to the original location, and made more generic, i.e. don't mention |gStateLock| by name, instead just say "the global lock" or something like that.

@@ +823,5 @@
> +// the shared library loader.  Another thread might call malloc
> +// while holding that lock (when loading a shared library).  So
> +// we have to exit gStateLock around this call.  For details, see
> +// https://bugzilla.mozilla.org/show_bug.cgi?id=363334#c3
> +struct UnlockState

Nit: |UnlockState| feels like the wrong name. Would |LocationLock| or |LocationLockState| be better? Not sure.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8464197 [details] [diff] [review]
part 7 - Move LocationService into xpcom.

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

At first I was concerned that DMD would now depend on XPCOM, but I guess all the code is in the header so it's just a #include dependence, not a linking dependence, just like js/public/HashTable.h. So I guess that should be ok. Maybe a comment about this at the #include point would be helpful.

Comment 20

[Nicholas Nethercote [inactive]]

Thanks for breaking up the change into small pieces. If you're using |hg mq| my minor suggestions might cause patch conflicts, in which case modifying the remaining patch files themselves might help avoid those conflicts.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8464196 [details] [diff] [review]
part 6 - Add empty LocationService.h file.

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

With my bikeshedding hat on, I think CodeLocationService is a slightly better name for this, for symmetry with NS_DescribeCodeAddress.  |LocationService| makes me think of geolocation services.  Or perhaps CodeAddressService?

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 8464197 [details] [diff] [review]
part 7 - Move LocationService into xpcom.

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

njn's comments about comments pertaining to #include dependence are good.  Bikeshedding comments still apply.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8464198 [details] [diff] [review]
part 8 - Implement a caching version of WalkTheStack using LocationService.

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

r=me with either a satisfactory "yes, there is this excellent reason" answer or "no, I will make the obvious substitution".

::: xpcom/base/nsTraceRefcnt.cpp
@@ +967,5 @@
>  
>  }
>  
>  void
>  nsTraceRefcnt::WalkTheStack(FILE* aStream)

Is there any reason to make the stackwalker not always use caching?

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd (:froydnj) from comment #23)
> Is there any reason to make the stackwalker not always use caching?

Yeah, see comment 14: "This only uses the cached version for refcounting, because the location service uses a lot of memory, so it is overkill if you just want to call it once for NS_ASSERTION.  The acute reason for not doing it is that in fact Mochitests blow up on TBPL when it tries to allocate the location service."
We OOM on TBPL, basically.

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> (In reply to Nathan Froyd (:froydnj) from comment #23)
> > Is there any reason to make the stackwalker not always use caching?
> 
> Yeah, see comment 14: "This only uses the cached version for refcounting,
> because the location service uses a lot of memory, so it is overkill if you
> just want to call it once for NS_ASSERTION.  The acute reason for not doing
> it is that in fact Mochitests blow up on TBPL when it tries to allocate the
> location service."
> We OOM on TBPL, basically.

I completely fail at reading submission comments.  Would you please add a comment to this effect, probably around the declaration of WalkTheStackCached?

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: part 9 - Rename DMDLocationService to CodeAddressService.

I split this into a separate patch to minimize rebasing headaches.

Comment 27

[Andrew McCreight [:mccr8]]

> Or perhaps CodeAddressService?
I renamed LocationService to CodeAddressService.

Comment 28

[Andrew McCreight [:mccr8]]

I added the various missing | in that one comment.

> Can you make this a class instead of a struct?

Done.

I renamed UnlockState to DescribeCodeAddressLock, and moved the comment back to where it was (making it more generic.  I added a comment to where DMD includes CodeAddressService that talks about the linkage.

Finally, I added a comment to WalkTheStackCached about the difference between it and WalkTheStack, and when you should use it. That was a good point...

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/810152d73f60
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f44880e80c05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e999a2c92ea1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e95f356891
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/949c2636d2d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8e112fe6de77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0739a2f246
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6bdab42ab879
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c35f4382d0eb

Comment 29

[Andrew McCreight [:mccr8]]

Looks like I missed one place that needed a STACKWALKING_AVAILABLE guard:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cf5734aff2

Comment 30

[Andrew McCreight [:mccr8]]

I backed out that change and part 8 because it breaks S builds somehow.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32151a649e11
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/46d371e3875c

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/810152d73f60
https://hg.mozilla.org/mozilla-central/rev/f44880e80c05
https://hg.mozilla.org/mozilla-central/rev/e999a2c92ea1
https://hg.mozilla.org/mozilla-central/rev/d8e95f356891
https://hg.mozilla.org/mozilla-central/rev/949c2636d2d6
https://hg.mozilla.org/mozilla-central/rev/8e112fe6de77
https://hg.mozilla.org/mozilla-central/rev/9b0739a2f246
https://hg.mozilla.org/mozilla-central/rev/c35f4382d0eb

Comment 32

[Andrew McCreight [:mccr8]]

Turns out nsTraceRefcnt.cpp is getting a ton of headers bootlegged in via unified builds.  We should also put a "using mozilla" at the top, as it is picking that up from somewhere, so as to reduce the chances for nonunified build bustage in the future.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3990d1c42ff

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3990d1c42ff

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/810152d73f60
https://hg.mozilla.org/mozilla-central/rev/f44880e80c05
https://hg.mozilla.org/mozilla-central/rev/e999a2c92ea1
https://hg.mozilla.org/mozilla-central/rev/d8e95f356891
https://hg.mozilla.org/mozilla-central/rev/949c2636d2d6
https://hg.mozilla.org/mozilla-central/rev/8e112fe6de77
https://hg.mozilla.org/mozilla-central/rev/9b0739a2f246
https://hg.mozilla.org/mozilla-central/rev/c35f4382d0eb