Closed
Bug 1296875
Opened 8 years ago
Closed 8 years ago
TraceLogger: minor refactoring
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
Details
Attachments
(1 file)
22.81 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
I wanted to get familiar with the TraceLogger code, and founds some opportunities for minor refactoring and commenting changes.
Assignee | ||
Comment 1•8 years ago
|
||
Note that the refcount ordering is for the case where you are self-assigning with a starting refcount of 1.
Attachment #8783211 -
Flags: review?(hv1989)
Comment 2•8 years ago
|
||
Comment on attachment 8783211 [details] [diff] [review] Minor TraceLogger cleanups, comments, refactoring Review of attachment 8783211 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Much appreciated. ::: js/src/vm/Debugger.cpp @@ -4823,5 @@ > return false; > > uint32_t textId = TLStringToTextId(linear); > > - if (!TLTextIdIsToggable(textId)) { Until now never noticed this was wrongly spelled! ::: js/src/vm/TraceLogging.cpp @@ +357,5 @@ > } > > AutoTraceLog internal(this, TraceLogger_Internal); > > + char* str = js_strdup(text); Better indeed @@ +1016,2 @@ > if (hasPayload()) > payload()->release(); Though it won't make a difference (in this code), since we don't actively remove the item when it hits 0, it is more logical like this indeed. ::: js/src/vm/TraceLoggingGraph.cpp @@ +33,5 @@ > using mozilla::NativeEndian; > > TraceLoggerGraphState* traceLoggerGraphState = nullptr; > > +#define MAX_LOGGERS 999 Can you add comment that if this get increased the %d of AllocTraceLogFilename alse needs to get adjusted? @@ +166,2 @@ > return false; > + auto cleanupTree = MakeScopeExit([&] { fclose(treeFile); treeFile = nullptr; }); Nice usage of MakeScopeExit!
Attachment #8783211 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #2) > ::: js/src/vm/TraceLoggingGraph.cpp > @@ +33,5 @@ > > using mozilla::NativeEndian; > > > > TraceLoggerGraphState* traceLoggerGraphState = nullptr; > > > > +#define MAX_LOGGERS 999 > > Can you add comment that if this get increased the %d of > AllocTraceLogFilename alse needs to get adjusted? Sure, though I'll have to do that in the next patch since AllocTraceLogFilename doesn't exist yet.
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b45dd84cc08 Minor TraceLogger cleanups, comments, refactoring; r=h4writer
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0afd311c614 Minor TraceLogger cleanups, comments, refactoring; r=h4writer
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0afd311c614
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•