Closed Bug 1296875 Opened 4 years ago Closed 3 years ago

TraceLogger: minor refactoring

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(1 file)

I wanted to get familiar with the TraceLogger code, and founds some opportunities for minor refactoring and commenting changes.
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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/d0afd311c614
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.