1779792 - Add IPC message identity fields to IPC profiler markers

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P3)

Description

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Currently 5 profiler markers are being combined to represent an IPC message in the profiler front-end. This is where we are doing this correlation.

To match IPC markers that belongs to a message, we are using a combination of various fields. Currently we use: senderPid,receiverPid,messageSeqno,messageType. This is unfortunately not enough because multiple threads from a process can send a message to another process. Also we use background thread sometimes that makes it harder to figure out.

While discussing this with the IPC folks, they recommended us to use a combination of different fields instead: messageSeqno + reply bit + actor pair identity. We have messageSeqno in the marker already and we can easily add the reply bit. But we don't have a actor pair identity in the IPC backend currently. That's why the biggest task here is this. We need to add this actor pair identity into the IPC codebase first and then add this information to our markers. Adding this information is a bit tricky since it needs to be kept updated across all the processes.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey Nika. We've discussed about this a while back and you were positive about helping me on this. Would it be okay to help me when you'll have some time? I know you are pretty busy and there is no rush, so I'm adding a needinfo to not ping you frequently.

Comment 2

[Julien Wajsberg [:julienw]]

Thanks for filing this bug Nazim!

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #0)

Currently 5 profiler markers are being combined to represent an IPC message in the profiler front-end. This is where we are doing this correlation.

To match IPC markers that belongs to a message, we are using a combination of various fields. Currently we use: senderPid,receiverPid,messageSeqno,messageType. This is unfortunately not enough because multiple threads from a process can send a message to another process. Also we use background thread sometimes that makes it harder to figure out.

Reading this, I wonder why we don't use tid instead of pid, is that not possible?

While discussing this with the IPC folks, they recommended us to use a combination of different fields instead: messageSeqno + reply bit + actor pair identity. We have messageSeqno in the marker already and we can easily add the reply bit. But we don't have a actor pair identity in the IPC backend currently. That's why the biggest task here is this. We need to add this actor pair identity into the IPC codebase first and then add this information to our markers. Adding this information is a bit tricky since it needs to be kept updated across all the processes.

It's not clear to me why messageSeqno with the other fields doesn't make the combination unique. How is messageSeqno generated and in which context could it be considered unique? Said differently: is there some fields that, added to messageSeqno, would make the combination unique, and if not why?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Julien Wajsberg [:julienw] from comment #2)

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #0)

While discussing this with the IPC folks, they recommended us to use a combination of different fields instead: messageSeqno + reply bit + actor pair identity. We have messageSeqno in the marker already and we can easily add the reply bit. But we don't have a actor pair identity in the IPC backend currently. That's why the biggest task here is this. We need to add this actor pair identity into the IPC codebase first and then add this information to our markers. Adding this information is a bit tricky since it needs to be kept updated across all the processes.

It's not clear to me why messageSeqno with the other fields doesn't make the combination unique. How is messageSeqno generated and in which context could it be considered unique? Said differently: is there some fields that, added to messageSeqno, would make the combination unique, and if not why?

The seqno makes the message unique going one way over a single toplevel actor, when combined with the reply bit, it makes it unique going over that toplevel actor (because a reply shares its seqno with the original message it's replying to). The last piece of information (actor pair identity) is the thing which would be required to make it unique globally. Right now we don't have an easy to access identifier indicating global actor pair identity, so we need to add something like it to make getting the full unique identity possible.

The reason why a seqno alone isn't enough is because we have many toplevel actors which can communicate over arbitrary pairs of content processes, and be created in arbitrary places, and passed around. There's a complex accounting system under the hood which makes sure that messages are routed to the right place, but it doesn't actually track actor pair identity (as each direction is different, and things like the destination process can change in real time).

In order to get an actor pair identity, we'd need to generate a new ID for each actor pair as its Endpoints are created, and pass them around along with the endpoint so we can assign it to the actor as it's created. In addition, we'd need to add some handling for each primary process actor to give it a globally unique ID, which might involve an additional command-line argument. To avoid having to check for conflicts, the easiest way is probably to use a UUID for the globally unique ID, though we can also probably use something inspired by nsContentUtils::GenerateProcessSpecificId if we want to limit ourselves to JS-safe uint64_ts (though we'll need to do something slightly different because that method only works in content processes right now, due to containing the ChildID).

Leaving my needinfo up so that I remember to come back to this after I'm back from PTO.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Update on this: been swamped with other things, but I haven't forgotten about it. I think my current rough idea is to take the somewhat-simple approach of adding a nsID id to every actor pair on the MessageChannel object. This is fairly straightforward to do for Endpoint-created channels, though will be more tricky for the initial process channel. I think there might be a simplification possible to how we create initial channels as well, which I might do first to make that part easier to implement (compared to adding flag reading code to every content process manually).

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 1: Use cached value for log ID generation, r=mccr8!

Previously this code read the atomic rather than the cached value (which
was unused). This is inherently racy as the atomic is updated on a
different thread than the read happened on.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 2: Use an Endpoint to bind the initial actor in child processes, r=#ipc-reviewers

This type is also used in other places to start non-initial actors, and will
allow us to attach additional state more easily without needing to thread it
through every child process callsite manually.

Depends on D153617

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 3: Use an endpoint to bind the initial actor in parent processes, r=#ipc-reviewers

This improves consistency with the child process case, and will make it easier
to attach additional state without needing to thread it through every child
process callsite manually.

Depends on D153618

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 4: Deduplicate ProcessChild subclass constructors, r=#ipc-reviewers

These constructors are unnecessary and can be defined with a using statement,
making it easier to change all constructors simultaneously.

Depends on D153619

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 5: Add a unique nsID field to each MessageChannel pair, r=#ipc-reviewers

This won't be used for any security or routing purposes, but can be useful for
debugging. It will be used in the future by the profiler to correlate sent and
received message events across processes.

Depends on D153620

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1779792 - Part 5: Add a unique nsID field to each MessageChannel pair, r=#ipc-reviewers

This won't be used for any security or routing purposes, but can be useful for
debugging. It will be used in the future by the profiler to correlate sent and
received message events across processes.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51821edf0162
Part 1: Use cached value for log ID generation, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/d9cc55d1f372
Part 2: Use an Endpoint to bind the initial actor in child processes, r=ipc-reviewers,necko-reviewers,media-playback-reviewers,mccr8,alwu
https://hg.mozilla.org/integration/autoland/rev/7cea00908fed
Part 3: Use an endpoint to bind the initial actor in parent processes, r=ipc-reviewers,necko-reviewers,media-playback-reviewers,alwu,mccr8
https://hg.mozilla.org/integration/autoland/rev/2ebc1accfd94
Part 4: Deduplicate ProcessChild subclass constructors, r=ipc-reviewers,necko-reviewers,media-playback-reviewers,alwu,mccr8
https://hg.mozilla.org/integration/autoland/rev/fb8b81db35d1
Part 5: Add a unique nsID field to each MessageChannel pair, r=ipc-reviewers,mccr8

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/51821edf0162
https://hg.mozilla.org/mozilla-central/rev/d9cc55d1f372
https://hg.mozilla.org/mozilla-central/rev/7cea00908fed
https://hg.mozilla.org/mozilla-central/rev/2ebc1accfd94
https://hg.mozilla.org/mozilla-central/rev/fb8b81db35d1

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Thanks a lot for working on this Nika!