913415 - Tracelogger: Add logging of background compiler thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Start of letting multiple threads log. For now limited to add logging for background compiler thread.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug913415-add-background-compile-logger

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.

Comment 2

[Till Schneidereit [: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?

Comment 3

[Hannes Verschore [:h4writer]]

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.

Comment 4

[Hannes Verschore [:h4writer]]

(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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/11b68cbcab1d