Tracelogger: Let it work in the browser again

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Since the introduction of e11s, we include "js" multiple times. As a result overwriting the tracelogger's graph data (tl-data.json ...). That is the reason TraceLogger doesn't work anymore in the browser.

As a solution I added the "pid" in the filename. And let tl-data.json point to the last created tl-data.xxx.json file. That makes it possible to still refer to tl-data.json without a problem, instead of having to find the actual pid which was used to log the file. This has a nice coincidence that by default we will also point to the content process, since that is started later than the chrome process.
(Assignee)

Comment 1

3 years ago
Created attachment 8753806 [details] [diff] [review]
Patch
Assignee: nobody → hv1989
Attachment #8753806 - Flags: review?(bbouvier)
Comment on attachment 8753806 [details] [diff] [review]
Patch

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

Makes sense, thanks! I guess you've also updated the aggregation scripts to have an optional argument that is the PID of the file?

Also, I think this won't work in the long run: if I remember correctly, the idea is that for testing e10s, there's only one content process. But in production, there should be more (probably around the number of cores on the machine, I'd guess?). So it is fine to make it work as is, as long as the aggregator can take an optional PID argument. But if you rely on tl-data.json giving you the right content process PID file, You're Gonna Have A Bad Time (c).

::: js/src/vm/TraceLoggingGraph.cpp
@@ +7,5 @@
>  #include "vm/TraceLoggingGraph.h"
>  
> +#ifdef XP_WIN
> +#include <process.h>
> +#define getpid _getpid

Can you undef at the end of file, please?

@@ +47,5 @@
>  
> +    // Write the last tl-data.*.json file to tl-data.json.
> +    // In most cases that is the wanted file.
> +    FILE* last = fopen(TRACE_LOG_DIR "tl-data.json", "w");
> +    if (last) {

if you'd want to, you could write it in a single line to minimize last's scoping: if (FILE* last = ...)
Attachment #8753806 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8753806 [details] [diff] [review]
> Patch
> 
> Review of attachment 8753806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense, thanks! I guess you've also updated the aggregation scripts to
> have an optional argument that is the PID of the file?
> 
> Also, I think this won't work in the long run: if I remember correctly, the
> idea is that for testing e10s, there's only one content process. But in
> production, there should be more (probably around the number of cores on the
> machine, I'd guess?). So it is fine to make it work as is, as long as the
> aggregator can take an optional PID argument. But if you rely on
> tl-data.json giving you the right content process PID file, You're Gonna
> Have A Bad Time (c).

Definitely. Ones has to provide the name of the file to the e.g. aggregation scripts. Therefore it is still possible to provide the name which includes the "pid". The default tl-data.json, just makes it easier that you don't always have to look at the directory to find the correct pid.

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10a48aecf994
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

2 years ago
Blocks: 1298831
You need to log in before you can comment on or make changes to this bug.