Closed Bug 1296876 Opened 4 years ago Closed 4 years ago

TraceLogger: write files to $TLDIR

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(2 files, 1 obsolete file)

My /tmp directory keeps getting cluttered with multiple batches of tracelogger outputs, and it's hard to keep straight which is which. It would be nice to be able to pick an output directory.
Attachment #8783212 - Flags: review?(hv1989)
Oops, one hunk was in the wrong patch.
Attachment #8783213 - Flags: review?(hv1989)
Attachment #8783212 - Attachment is obsolete: true
Attachment #8783212 - Flags: review?(hv1989)
Comment on attachment 8783213 [details] [diff] [review]
TraceLogger: write files to $TLDIR

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

Nice! Could you also document this on https://github.com/h4writer/tracelogger ?

::: js/src/vm/TraceLoggingGraph.cpp
@@ +54,5 @@
> +            p++;
> +            if (*p == 'u')
> +                len += sizeof("4294967295") - 1;
> +            else if (*p == 'd')
> +                len += sizeof("999") - 1;

Can we add asserts that check this? If possible that might be good to make sure %d doesn't get confused as being any int32 value.
Attachment #8783213 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8783213 [details] [diff] [review]
> TraceLogger: write files to $TLDIR
> 
> Review of attachment 8783213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! Could you also document this on
> https://github.com/h4writer/tracelogger ?

Yeah, I'll send you a PR for this and for displaying the thread names.

> ::: js/src/vm/TraceLoggingGraph.cpp
> @@ +54,5 @@
> > +            p++;
> > +            if (*p == 'u')
> > +                len += sizeof("4294967295") - 1;
> > +            else if (*p == 'd')
> > +                len += sizeof("999") - 1;
> 
> Can we add asserts that check this? If possible that might be good to make
> sure %d doesn't get confused as being any int32 value.

I don't know how, because the actual values are passed through varargs. At least without reimplementing a printf format string parser.

Oh! I guess I could at least check whether the final vsnprintf'ed string ended up too long. "Guess what? A second ago, you just trashed some random memory!"
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe209e9ca79
TraceLogger: write files to $TLDIR, r=h4writer
Attached patch Few fixesSplinter Review
Some small fixes I didn't see while reviewing.
Attachment #8784411 - Flags: review?(sphink)
Comment on attachment 8784411 [details] [diff] [review]
Few fixes

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

Yeah, I tried actually using it yesterday, and ended up with the same fixes. If it's ok, I'll just land my current patch, which already incorporates what you have here.

My TL-related patch stack is waiting on a review in bug 1296878, though.
Attachment #8784411 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink] [:s:] from comment #7)
> Comment on attachment 8784411 [details] [diff] [review]
> Few fixes
> 
> Review of attachment 8784411 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I tried actually using it yesterday, and ended up with the same fixes.
> If it's ok, I'll just land my current patch, which already incorporates what
> you have here.
> 
> My TL-related patch stack is waiting on a review in bug 1296878, though.

Sure
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c12dbf24cac
TraceLogger: write files to $TLDIR, r=h4writer
https://hg.mozilla.org/mozilla-central/rev/1c12dbf24cac
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.