Closed
Bug 1334409
Opened 7 years ago
Closed 7 years ago
Add memory reporter for Tracelogger
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(2 files)
15.96 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Whiteboard: [MemShrink]
Updated•7 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f26444a34566
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•