Closed Bug 1049051 Opened 5 years ago Closed 5 years ago

Templatize DeadlockDetector on BlockingResourceBase

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(6 files)

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
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
Summary: Detemplatize DeadlockDetector → Templatize DeadlockDetector on BlockingResourceBase
This removes the ResourceAcquisition type which was only wrapping a T*.
Attachment #8469668 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.
Attachment #8469671 - Flags: review?(nfroyd)
This just clarifies that DeadlockDetector does not mess with the entry type.
Attachment #8469674 - Flags: review?(nfroyd)
PrintCycle is moved to be a file scope static. This helps in the goal of just forward declaring DeadlockDetector in the header.
Attachment #8469676 - Flags: review?(nfroyd)
All access of sDeadlockDetector is moved to the implementation file and DeadlockDetector is forward declared.
Attachment #8469677 - Flags: review?(nfroyd)
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.
Attachment #8469668 - Flags: review?(nfroyd) → review+
Attachment #8469671 - Flags: review?(nfroyd) → review+
Attachment #8469674 - Flags: review?(nfroyd) → review+
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.
Attachment #8469676 - Flags: review?(nfroyd) → review+
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|.
Attachment #8469677 - Flags: review?(nfroyd) → review+
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
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.
Attachment #8471748 - Flags: review?(khuey)
Attachment #8471748 - Flags: review?(khuey) → review+
You need to log in before you can comment on or make changes to this bug.