1273855 - Tracelogger: Let it work in the browser again

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

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 = ...)

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a48aecf994

Comment 4

[Hannes Verschore [:h4writer]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10a48aecf994