Closed Bug 913415 Opened 6 years ago Closed 6 years ago

Tracelogger: Add logging of background compiler thread

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file)

Start of letting multiple threads log. For now limited to add logging for background compiler thread.
This logs in /tmp/tracelogging-compile.log whenever the background thread is compiling.

I'll add the generate.py changes as soon as possible. Today or Monday. But this can already get in. It doesn't break current behaviour.
Assignee: general → hv1989
Attachment #800712 - Flags: review?(till)
Comment on attachment 800712 [details] [diff] [review]
bug913415-add-background-compile-logger

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

Nice, r=me with nits addressed.

We should maybe do something similar for GC work that happens off the main thread, so it's nice that this is easily extendable. Looking forward to seeing what you come up with for the visualization. :)

::: js/src/TraceLogging.cpp
@@ +196,5 @@
>  void
>  TraceLogging::flush()
>  {
>      // Open the logging file, when not opened yet.
> +    if (out == NULL) {

"if (!out)". I was told various times that we don't do "== NULL".

@@ +242,2 @@
>          if (written < 0) {
> +            fprintf(stderr, "Writting to log file failed");

"Writing". Also, being a bit more specific might be nice. Something like "Writing tracelog to disk failed, probably because the file would've exceeded the maximum size of 2GB."

::: js/src/TraceLogging.h
@@ +94,3 @@
>      static const char * const type_name[];
> +    static TraceLogging* loggers[];
> +    static uint64_t startupTime;

This should probably be startup_time, right?
Attachment #800712 - Flags: review?(till) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b68cbcab1d

(In reply to Till Schneidereit [:till] from comment #2)
> We should maybe do something similar for GC work that happens off the main
> thread, so it's nice that this is easily extendable. Looking forward to
> seeing what you come up with for the visualization. :)
Good call. Didn't this would be another thread that would be really helpful to add.

> 
> ::: js/src/TraceLogging.cpp
> @@ +196,5 @@
> >  void
> >  TraceLogging::flush()
> >  {
> >      // Open the logging file, when not opened yet.
> > +    if (out == NULL) {
> 
> "if (!out)". I was told various times that we don't do "== NULL".

Yeah, I removed all checks with NULL.

> ::: js/src/TraceLogging.h
> @@ +94,3 @@
> >      static const char * const type_name[];
> > +    static TraceLogging* loggers[];
> > +    static uint64_t startupTime;
> 
> This should probably be startup_time, right?

Oops, looks like I used different code styles. We want camel Case. So changed all blaat_blaat to blaatBlaat.
(In reply to Hannes Verschore [:h4writer] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/11b68cbcab1d
> 
> (In reply to Till Schneidereit [:till] from comment #2)
> > We should maybe do something similar for GC work that happens off the main
> > thread, so it's nice that this is easily extendable. Looking forward to
> > seeing what you come up with for the visualization. :)
> Good call. Didn't this would be another thread that would be really helpful
> to add.

I'll translate:
Right, forgot it would be really helpful and easy to add GC too.
https://hg.mozilla.org/mozilla-central/rev/11b68cbcab1d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.