Closed Bug 739681 Opened 9 years ago Closed 8 years ago

Allow DumpHeapComplete to print unreachable objects


(Core :: JavaScript Engine, defect)

Not set



blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed


(Reporter: billm, Assigned: billm)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
In some cases it would be nice to be able to print out the entire JS heap, including unreachable objects. This patch adds options to that allow that to happen.
Attachment #609767 - Flags: review?(continuation)
Blocks: 723783
Comment on attachment 609767 [details] [diff] [review]

Review of attachment 609767 [details] [diff] [review]:

Overall, it looks good to me.  As a bonus, this patch makes the whole root marking story less weird.

r- because I'd like to see the refactored version of DumpHeapComplete.

::: js/src/jsfriendapi.cpp
@@ +523,3 @@
>      Vector<DumpingChildInfo, 0, SystemAllocPolicy> nodes;
> +    char buffer[200];
> +    js::DumpSelector whatToDump;

A DumpSelector looks like overkill here, as all you really need is a boolean that says whether you want to visit children or not.  But I guess this is more general for possible future changes, and space is completely irrelevant for this.  So either way.

@@ +549,5 @@
>      dtrc->nodes.append(DumpingChildInfo(thing, kind));
>  }
>  static void
> +DumpHeapRootFunc(JSTracer *trc, void **thingp, JSGCTraceKind kind)

Maybe DumpHeapVisitRoot?  Saying that a function is a function isn't very useful. ;)

@@ +566,3 @@
>      JSDumpHeapTracer *dtrc = static_cast<JSDumpHeapTracer *>(trc);
>      const char *edgeName = JS_GetTraceEdgeName(dtrc, dtrc->buffer, sizeof(dtrc->buffer));
> +    fprintf(dtrc->output, "> %p %s\n", *thingp, edgeName);

Ah, good you got rid of the color here.  I was meaning to get around to doing that myself at some point.

@@ +574,5 @@
> +static void
> +DumpHeapCompartmentFunc(JSRuntime *rt, void *data, JSCompartment *compartment)
> +{
> +    JSDumpHeapTracer *dtrc = static_cast<JSDumpHeapTracer *>(data);
> +    const char *name = "<unknown>";

So every non-atoms compartment becomes "unknown"?  It doesn't seem very useful to me to print that, but if this is some JS engine convention that's fine.

@@ +582,5 @@
> +}
> +
> +static void
> +DumpHeapArenaFunc(JSRuntime *rt, void *data, gc::Arena *arena,
> +                          JSGCTraceKind traceKind, size_t thingSize)

The alignment on these arguments looks off.

@@ +591,5 @@
> +}
> +
> +static void
> +DumpHeapCellFunc(JSRuntime *rt, void *data, void *thing,
> +                         JSGCTraceKind traceKind, size_t thingSize)

The alignment on these arguments looks off.

@@ +606,2 @@
>  {
> +    JSDumpHeapTracer dtrc(fp, whatToDump);

All of these weirdly overlapping control paths are confusing and they don't seem to buy you much.  I think you should split it into two cases at the top level, one for DUMP_REACHABLE and one for two kinds of DUMP_ALLOCATED.  You could even make each case its own function, though these are simple enough isn't necessary.

@@ -602,3 @@
>      }
> -    dtrc.visited.finish();

I notice the finish went away.  I guess that isn't needed?  I think I cargo culted it from somewhere.

::: js/src/jsfriendapi.h
@@ +231,5 @@
>  #ifdef DEBUG
> +enum DumpSelector {

This is kind of nitpicky, but it seems a little weird that DUMP_REACHABLE also dumps roots, but doesn't say that it does.  Maybe you could change DUMP_ALLOCATED to DUMP_ALLOCATED_NO_ROOTS and WITH_ROOTS to just DUMP_ALLOCATED?  It is sort of implied that reachable things relate to roots, but I could imagine not dumping them, for some reason...
Attachment #609767 - Flags: review?(continuation) → review-
Attached patch patch v2Splinter Review
This simplifies things a lot. I also added a function to call this thing from chrome code or the shell.
Attachment #609767 - Attachment is obsolete: true
Attachment #677510 - Flags: review?(continuation)
Comment on attachment 677510 [details] [diff] [review]
patch v2

Review of attachment 677510 [details] [diff] [review]:

Very nice!

::: js/src/jsfriendapi.cpp
@@ +642,5 @@
> +DumpHeapVisitCell(JSRuntime *rt, void *data, void *thing,
> +                  JSGCTraceKind traceKind, size_t thingSize)
> +{
> +    JSDumpHeapTracer *dtrc = static_cast<JSDumpHeapTracer *>(data);
> +    char buffer[1024];

nit: please give |buffer| a less generic name, like |descr| or |thingInfo|

@@ +658,5 @@
> +            JS_GetTraceEdgeName(dtrc, buffer, sizeof(buffer)));
> +}
> +
> +static void
> +DumpHeapVisitRoot(JSTracer *trc, void **thingp, JSGCTraceKind kind)

Kind of a shame that this an DumpHeapVisitChild are almost identical, but oh well...
Attachment #677510 - Flags: review?(continuation) → review+
(the other uses of the variable name "buffer" look fine to me)
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
We needed this to fix bug 893012 (applied locally to khuey's build), so it should probably get landed on b2g18, in case of similar problems in the future.  This is very low risk, because it only changes code that runs when we dump logs, and it has been on trunk since November.
Blocks: 893012
blocking-b2g: --- → leo?
Go for it, but note that leo is cherry-picking, so we can't guarantee anything.
blocking-b2g: leo? → leo+
You need to log in before you can comment on or make changes to this bug.