Add the process id to the thread

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gregtatum, Assigned: gregtatum)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

2 years ago
Assignee: nobody → gtatum
(Assignee)

Comment 1

a year ago
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?
Flags: needinfo?(mstange)
Flags: needinfo?(jimb)
(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.
Flags: needinfo?(mstange)
(Assignee)

Comment 3

a year ago
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.
Flags: needinfo?(jimb)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8850027 [details]
Bug 1346776 - Add the process id in the Gecko Profiler;

https://reviewboard.mozilla.org/r/122776/#review124946
Attachment #8850027 - Flags: review?(mstange) → review+

Comment 6

a year ago
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f81dc0fe12d2
Add the process id in the Gecko Profiler; r=mstange

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f81dc0fe12d2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.