Tracelogger: Add logging of background compiler thread

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Start of letting multiple threads log. For now limited to add logging for background compiler thread.
(Assignee)

Comment 1

4 years ago
Created attachment 800712 [details] [diff] [review]
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.
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+
(Assignee)

Comment 3

4 years ago
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.
(Assignee)

Comment 4

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.