1346776 - Add the process id to the thread

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Greg Tatum [:gregtatum]]

Now that bug 1333277 has landed, we have the process id for particular tabs. For perf.html we want to be able to capture that information:

https://github.com/devtools-html/perf.html/issues/134

Comment 1

[Greg Tatum [:gregtatum]]

I'm working on the PID, and Markus and I had some chat discussion surrounding the PID. I'd like to get some clarify on direction for the profile. Here is our chat log:

> mstange:
> I think it would make more sense to put the pid on profile.meta than on each thread. When I added the processType string, I added it on the thread, and I didn't realize that we already had a processType field in profile.meta (but that one is an integer...)
> 
> gregtatum:
> ah hmm... but can't the pid be different per thread? I was assuming the profiler worked across processes
> 
> mstange:
> ah, I should clarify 
> 
> mstange:
> actually, I'm not even sure what a good API for this would look like
> 
> mstange:
> so in the raw profile format, every "process" will have its own meta object
> 
> mstange:
> but you're right, in the preprocessed profile, we lose that information by collecting threads from all processes into one single threads array

Currently the structure looks like this:

> profile {
>  meta: {
>     // This will be the process where the profile was collected.
>     processType,
>     ...
>   }
>   threads: [
>     { processType, ... },
>     { processType, ... }
>   ]
> }

The easiest fix would be in the raw Gecko Profile format add per-thread process information:

> profile = {
>   meta: {
>     // Profile-only meta, do we even care about the process?
>     interval: 1,
>     abi: "x86_64-gcc3"
>     ...
>   }
>   threads: [
>     { processType, pid, ... },
>     { processType, pid, ... }
>   ]
> }

Markus seems to be proposing a structure that looks like the following:

> profile = {
>   meta: {
>     // Profile-only meta.
>     interval: 1,
>     abi: "x86_64-gcc3"
>     ...
>   }
>   processes: {
>     {
>       meta: { processType, pid, ... },
>       thread: [ { ... }, { ... } ],
>     },
>     {
>       meta: { processType, pid, ... },
>       thread: [ { ... }, { ... } ],
>     }
>   }
> }

What's the best path forward here? Do the easy patch in the Gecko Profiler and add the PID per thread? Or do a big patch moving the data structure around, collecting the threads per process. This will break devtools and require additional patches. Or the second easiest, do the easy platform patch, and then refactor the data structure on the client only? Ehsan has a clear right now and it would be good to deliver a solution soon.

Maybe it would be better to address the structure as follow-ups?

Comment 2

[Markus Stange [:mstange]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #1)
> What's the best path forward here? Do the easy patch in the Gecko Profiler
> and add the PID per thread?

Yes. Please don't let my future plans for the format slow you down.

Now that we have a clear profile versioning plan, it's easy to make changes to the format any time we like, we don't have to get it right from the start.

> Or the second easiest, do the easy platform
> patch, and then refactor the data structure on the client only?

This doesn't seem worth doing at the moment.

Comment 3

[Greg Tatum [:gregtatum]]

Clearing NI from Jim (feel free to weigh in still) but there is bug 1329114 on this change as well. We'll also be more free to change things once we are only supporting one client. I'll move forward with the quick solution.

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: Bug 1346776 - Add the process id in the Gecko Profiler;

I also tweaked the int type of the JSON writer to match the IntProperty
method.

Review commit: https://reviewboard.mozilla.org/r/122776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122776/

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8850027 [details]
Bug 1346776 - Add the process id in the Gecko Profiler;

https://reviewboard.mozilla.org/r/122776/#review124946

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f81dc0fe12d2
Add the process id in the Gecko Profiler; r=mstange

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f81dc0fe12d2