Closed Bug 1004609 Opened 7 years ago Closed 7 years ago

Use DMD's LocationService for nsTraceRefcnt's refcount logging

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 3 obsolete files)

1.87 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
8.88 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
6.03 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
3.28 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
3.66 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
1.27 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
15.17 KB, patch
n.nethercote
: review+
froydnj
: review+
Details | Diff | Splinter Review
9.67 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
7.08 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
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.
Oops forget to include the new file.
Attachment #8416023 - Attachment is obsolete: true
Assignee: nobody → continuation
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.
Flags: needinfo?(nfroyd)
Attached patch WIP, folded together (obsolete) — Splinter Review
Attachment #8416027 - Attachment is obsolete: true
(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.
(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.
Flags: needinfo?(nfroyd)
Depends on: 1045239
Depends on: 1045241
Version: 28 Branch → Trunk
This will make it easier to move it later, and is barely used in the LocationService.
Attachment #8464190 - Flags: review?(n.nethercote)
I just split this out to make rebasing over changes to the location service easier.
Attachment #8464196 - Flags: review?(nfroyd)
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.
Attachment #8431787 - Attachment is obsolete: true
Attachment #8464197 - Flags: review?(nfroyd)
Attachment #8464197 - Flags: review?(n.nethercote)
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.
Attachment #8464198 - Flags: review?(nfroyd)
Summary: Make nsTraceRefcnt::WalkTheStack use DMD's LocationService → Use DMD's LocationService for nsTraceRefcnt's refcount logging
Attachment #8464190 - Flags: review?(n.nethercote) → review+
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.
Attachment #8464191 - Flags: review?(n.nethercote) → review+
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|".
Attachment #8464192 - Flags: review?(n.nethercote) → review+
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.
Attachment #8464193 - Flags: review?(n.nethercote) → review+
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.
Attachment #8464194 - Flags: review?(n.nethercote) → review+
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.
Attachment #8464197 - Flags: review?(n.nethercote) → review+
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 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?
Attachment #8464196 - Flags: review?(nfroyd) → review+
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.
Attachment #8464197 - Flags: review?(nfroyd) → review+
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?
Attachment #8464198 - Flags: review?(nfroyd) → review+
(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.
(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?
I split this into a separate patch to minimize rebasing headaches.
Attachment #8465746 - Flags: review?(n.nethercote)
> Or perhaps CodeAddressService?
I renamed LocationService to CodeAddressService.
Attachment #8465746 - Flags: review?(n.nethercote) → review+
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
Looks like I missed one place that needed a STACKWALKING_AVAILABLE guard:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cf5734aff2
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
Keywords: leave-open
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/a3990d1c42ff
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.