Closed Bug 890135 Opened 11 years ago Closed 11 years ago

Switch B2G's "gc log" dump to produce an all-traces log

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

(Whiteboard: [MemShrink][LeoVB+])

Attachments

(2 files, 1 obsolete file)

Right now when you do

  echo "gc log" > /data/local/debug_info_trigger

the logs we produce are not all-traces logs.  This makes debugging unnecessarily difficult.

I don't see any reason not to always produce all-traces logs.  If there is a reason, we can definitely make it optional.

Patch coming.
Whiteboard: [MemShrink]
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #771142 - Flags: review?(continuation)
Have you tested this out?  The CC logs are going to be much bigger, plus slightly slower because they run an extra GC.  I don't know if that matters or not.  GC logs will be the same size.
Yes, I used it in bug 889984.  It was super helpful.

The only concern I'd have about the logs being bigger is that we might run out of space in /data/local/tmp on the phone.  That doesn't seem to be happening right now, but if we get into that situation, we can always compress the logs while we write them out.
Attachment #771142 - Flags: review?(continuation) → review+
For me the non-all-trace logs have been more useful. allTrace logs just occasionally just too
huge to analyze easily, and non-alltraces tend to contain the useful stuff - of course that depends
on the case.
It depends on what kind of leak it is.  In this particular case, bug 889984, the leaked objects were being held alive by a window, so AllTraces was needed to get the full story.  It would be nice to have both, but this is just for B2G so Justin can adjust it as he likes.
If you've seen circumstances in which the non-all-trace logs are useful, I'll add it -- it's easy enough to write a patch to make that work.  It's better to err on the side of more options, because when you hit the leak, it's too late to rebuild.
kats, johns: Can I change nsIMemoryInfoDumper::dumpGCAndCCLogsToFile() to include a new boolean argument?

I don't think either of you is using that function, but I wanted to check.
Flags: needinfo?(jschoenick)
Flags: needinfo?(bugmail.mozilla)
(In reply to Justin Lebar [:jlebar] from comment #7)
> I don't think either of you is using that function, but I wanted to check.

AWSY desktop, at least, is not using any of the dump-to-file functions
Flags: needinfo?(jschoenick)
AWSY mobile is not using that function either, so go right ahead. Thanks for checking!
Flags: needinfo?(bugmail.mozilla)
Attached patch Patch, v2Splinter Review
Attachment #772058 - Flags: review?(continuation)
Attachment #771142 - Attachment is obsolete: true
B2G changes, which I'll merge after we land this:

https://github.com/jlebar/B2G/tree/fix-get-gc-cc-log
Comment on attachment 772058 [details] [diff] [review]
Patch, v2

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

::: xpcom/base/nsIMemoryInfoDumper.idl
@@ +133,5 @@
>     *
> +   * @param aDumpAllTraces indicates whether we should run an all-traces CC log.
> +   *   An all-traces log visits all objects in the CC heap, while a
> +   *   non-all-traces log avoids visiting some objects which we know are
> +   *   reachable.

It might be a little confusing to say "all objects" here, given that all all-traces log may not actually visit all CCed objects that are in the heap. Maybe you could just add a disclaimer to that effect?

@@ +136,5 @@
> +   *   non-all-traces log avoids visiting some objects which we know are
> +   *   reachable.
> +   *
> +   *   All-traces logs are much bigger than the alternative, but they may be
> +   *   helpful when trying to understand why a particular object is alive.

It would be good to say when it is helpful. Maybe something like "For instance, an object that is being held alive by a currently displayed document" or whatever the correct terminology is there.
Attachment #772058 - Flags: review?(continuation) → review+
> It might be a little confusing to say "all objects" here, given that all all-traces log 
> may not actually visit all CCed objects that are in the heap.

What objects are and aren't covered by an all-traces log?
Actually, I'm going to merge my changes into B2G now, because I'm about to ask someone to take a gc log, and the tool doesn't work as-is.
(In reply to Justin Lebar [:jlebar] from comment #14)
> Actually, I'm going to merge my changes into B2G now, because I'm about to
> ask someone to take a gc log, and the tool doesn't work as-is.

Sounds good.

Yeah, it is hard to describe the set of objects visited by all traces.  I'll try to think of something...
Assignee: nobody → justin.lebar+bug
https://hg.mozilla.org/mozilla-central/rev/d87b950c7a6f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Zero risk, only run during debugging, necessary for leo blocker bug 893012.
blocking-b2g: --- → leo+
This doesn't cleanly apply, as b2g18 doesn't have the
  nsCOMPtr<nsIMemoryInfoDumper> dumper = do_GetService("@mozilla.org/memory-info-dumper;1");
stuff.  I'll try to figure out how to backport it I guess.
Whiteboard: [MemShrink] → [MemShrink][mccr8 will backport]
Attached patch patch, for b2g18Splinter Review
I haven't entirely made sure this finishes building.
Whiteboard: [MemShrink][mccr8 will backport] → [MemShrink]
Whiteboard: [MemShrink] → [MemShrink][LeoVB+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: