1049051 - Templatize DeadlockDetector on BlockingResourceBase

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Eric Rahm [:erahm]]

DeadlockDetector is currently templatized on it's entry. I'm sure there was a reason, but it's not being used in a useful way AFAICT. There is currently only one user [1] of the DeadlockDetector and one entry type [2].

This change would gain us a few benefits:
  * Cleaner code
  * We would be able to just forward decl it in BlockingResourceBase and hide the implementation details
  * Changes to DeadlockDetector would no longer require nearly full rebuilds

[1] http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/xpcom/glue/BlockingResourceBase.h#l60
[2] http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/xpcom/glue/BlockingResourceBase.h#l71

Comment 1

[Eric Rahm [:erahm]]

Full detemplatization probably isn't worth the effort.

Instead just do a subset of the work:
  * Just forward declare the DeadlockDetector and move implementation details into BlockingResourceBase
  * Remove DeadlockDetectorEntry and just use BlockingResourceBase directly
  * We can also remove ResourceAcquisition and just use BlockingResourceBase

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove ResourceAcquisition

This removes the ResourceAcquisition type which was only wrapping a T*.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 2: Remove DeadlockDetectorEntry

This removes a layer of indirection that is not needed and maybe saves a bit of memory. DeadlockDetector's members are merged into BlockingResourceBase, BRB is used as the entry type in DeadlockDetector, and DeadlockDetector no longer takes ownership of the entry.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 3: DeadlockDetector Add/Remove can take a const param

This just clarifies that DeadlockDetector does not mess with the entry type.

Comment 5

[Eric Rahm [:erahm]]

Attachment: Part 4: Make PrintCycle internal

PrintCycle is moved to be a file scope static. This helps in the goal of just forward declaring DeadlockDetector in the header.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Part 5: Make sDeadlockDetector access internal

All access of sDeadlockDetector is moved to the implementation file and DeadlockDetector is forward declared.

Comment 7

[Eric Rahm [:erahm]]

https://tbpl.mozilla.org/?tree=Try&rev=922888572b83

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8469668 [details] [diff] [review]
Part 1: Remove ResourceAcquisition

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

I'm going to trust that you have a plan that doesn't require re-introducing a lot of this stuff when callstack support is added back in.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8469676 [details] [diff] [review]
Part 4: Make PrintCycle internal

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

::: xpcom/glue/BlockingResourceBase.cpp
@@ +52,5 @@
> + * contexts into strings, all info is written to stderr, but only
> + * some info is written into |aOut|
> + */
> +bool
> +PrintCycle(const BlockingResourceBase::DDT::ResourceAcquisitionArray* aCycle, nsACString& aOut)

Is this in an anonymous namespace?  If not, this function should be |static|.

You may want to typedef |BlockingResourceBase::DDT::ResourceAcquisitionArray| outside this function and use the typedef within the function; would help the function stay within the 80-char line-length limit.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8469677 [details] [diff] [review]
Part 5: Make sDeadlockDetector access internal

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

::: xpcom/glue/BlockingResourceBase.cpp
@@ +160,5 @@
> +void
> +BlockingResourceBase::Shutdown()
> +{
> +  delete sDeadlockDetector;
> +  sDeadlockDetector = 0;

Might consider making |sDeadlockDetector| a StaticAutoPtr so we don't need the explicit |delete|.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest leaks. Don't forget to add the r= to these patches when they re-land too :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d90a2cce157

https://tbpl.mozilla.org/php/getParsedLog.php?id=45684150&tree=Mozilla-Inbound

Comment 12

[Ryan VanderMeulen [:RyanVM]]

In case you didn't see the comment I made in-channel, B2G mochitests aren't leak-free yet in normal runs, so this could be an instance where you introduced a small leak that pushed us over the allowable threshold. You might want to go through the list of leaked objects and see if there's anything sensible in there vs. looking at the list as a whole.

The dependencies filed against bug 1038943 may help in that effort too.

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 6: Adjust leak threshold

Comment 14

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9df58cfd576
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0ac45f3e0c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c9c9e41892
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1cbf66a170
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcb703c7ad2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd3e8a2cd09

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b9df58cfd576
https://hg.mozilla.org/mozilla-central/rev/3e0ac45f3e0c
https://hg.mozilla.org/mozilla-central/rev/c1c9c9e41892
https://hg.mozilla.org/mozilla-central/rev/fd1cbf66a170
https://hg.mozilla.org/mozilla-central/rev/0dcb703c7ad2
https://hg.mozilla.org/mozilla-central/rev/ffd3e8a2cd09

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/metro/rev/c4649e6ddd22
https://hg.mozilla.org/projects/metro/rev/f4fdedef6297
https://hg.mozilla.org/projects/metro/rev/96b54c1771b6
https://hg.mozilla.org/projects/metro/rev/06dd873bb3e9
https://hg.mozilla.org/projects/metro/rev/4a283d42050c