Closed Bug 1334409 Opened 7 years ago Closed 7 years ago

Add memory reporter for Tracelogger

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files)

      No description provided.
On P1 for myself
Priority: -- → P1
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Makes sure this isn't black matter anymore and we can see in the memory reporter next time tracelogger misbehaves. Which I hope will never happen again.
Assignee: nobody → hv1989
Attachment #8857989 - Flags: review?(n.nethercote)
Comment on attachment 8857989 [details] [diff] [review]
Add memory reporter

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

Looks good. You can verify the changes with DMD if you want: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD has instructions.

::: js/src/vm/Runtime.cpp
@@ +470,5 @@
>          rtSizes->atomsTable += permanentAtoms->sizeOfIncludingThis(mallocSizeOf);
>      }
>  
> +    rtSizes->tracelogger += TraceLogStateSizeOf(mallocSizeOf);
> +    rtSizes->tracelogger += TraceLogGraphStateSizeOf(mallocSizeOf);

The usual convention is to put SizeOf at the start of the name, which makes these SizeOfTraceLogState() and SizeOfTraceLogGraphState().

@@ +478,5 @@
>          rtSizes->contexts += mallocSizeOf(cx);
>          rtSizes->contexts += cx->sizeOfExcludingThis(mallocSizeOf);
>          rtSizes->temporary += cx->tempLifoAlloc().sizeOfExcludingThis(mallocSizeOf);
>          rtSizes->interpreterStack += cx->interpreterStack().sizeOfExcludingThis(mallocSizeOf);
> +        rtSizes->tracelogger += cx->traceLogger->sizeOfExcludingThis(mallocSizeOf);

cx->traceLogger is a pointer to a heap-allocated object, so this should be sizeOfIncludingThis() instead of sizeOfExcludingThis().

::: js/src/vm/TraceLogging.cpp
@@ +344,5 @@
> +TraceLoggerThreadState::sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf)
> +{
> +    LockGuard<Mutex> guard(lock);
> +
> +    // Do not count threadLoggers since they are counted by JSContext::traceLogger.

Good comment.

::: js/src/vm/TraceLogging.h
@@ +449,5 @@
> +
> +    size_t sizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf);
> +    size_t sizeOfIncludingThis(mozilla::MallocSizeOf mallocSizeOf) {
> +        return mallocSizeOf(this) + sizeOfExcludingThis(mallocSizeOf);
> +    }

Make these |const| if possible.

::: js/src/vm/TraceLoggingGraph.cpp
@@ +207,5 @@
> +js::TraceLogGraphStateSizeOf(mozilla::MallocSizeOf mallocSizeOf)
> +{
> +    if (traceLoggerGraphState)
> +        return traceLoggerGraphState->sizeOfIncludingThis(mallocSizeOf);
> +    return 0;

I'd use ?: here, but your taste may be different. Likewise for js::TraceLogStateSizeOf().
Attachment #8857989 - Flags: review?(n.nethercote) → review+
Attached patch FixesSplinter Review
After using DMD I noticed we were reporting the TraceLoggerState multiple times. This is the fix for to not report it multiple times.
Attachment #8859125 - Flags: review?(n.nethercote)
Comment on attachment 8859125 [details] [diff] [review]
Fixes

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

::: js/public/MemoryMetrics.h
@@ +965,5 @@
>  AddServoSizeOf(JSContext* cx, mozilla::MallocSizeOf mallocSizeOf,
>                 ObjectPrivateVisitor *opv, ServoSizes *sizes);
>  
> +extern JS_PUBLIC_API(bool)
> +CollectTraceLoggerStateStats(RuntimeStats *rtStats);

'*' to the left.

::: js/src/vm/MemoryMetrics.cpp
@@ +882,5 @@
> +JS::CollectTraceLoggerStateStats(RuntimeStats *rtStats)
> +{
> +    rtStats->runtime.tracelogger += TraceLogStateSizeOf(rtStats->mallocSizeOf_);
> +    rtStats->runtime.tracelogger += TraceLogGraphStateSizeOf(rtStats->mallocSizeOf_);
> +    return true;

Make the return type |void|?
Attachment #8859125 - Flags: review?(n.nethercote) → review+
Pushed by hv1989@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f26444a34566
TraceLogging: Add memory reporter for TraceLogging, r=njn
https://hg.mozilla.org/mozilla-central/rev/f26444a34566
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.