profile: Make the time and memory profilers run over IPC.

[pcwalton]

Uses a couple of extra threads to work around the lack of cross-process
boxed trait objects.

r? @nnethercote

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5552

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[nnethercote]

What follows is me trying to understand all this...

Let's call the memory profiler thread "M".

Old scheme: each thread, T, that does memory reporting has a Sender that
implements the Reporter trait so that M can send requests to T. When those
requests are received, the Reporter sends a thread-specific message to T to
collect reports.

New scheme: each thread, T, that does memory reporting has an auxiliary
reporter thread, R. A channel from M to R is established. R just sits waiting
for requests from the memory profiler thread. When those requests are received,
R sends a thread-specific message to T to collect reports.

In both schemes we require a thread-specific intermediary to convert the generic
"collect reports" request from M to the equivalent thread-specific message for
T. In the old scheme that's the trait, in the new one it's the thread. And in
both schemes these intermediaries end up looking very similar to each other.

Is there any way to use the type system to instead share the generic event
among multiple threads? I'm imagining some kind of enum inheritance. E.g. have
a ReporterMsg enum with a single variant, CollectReports, and then the layout
task's Msg type would inherit from it, and would the script task's ScriptMsg,
and so on. And M would send messages of type ReporterMsg.

That's probably not possible, but it seems a shame to have an extra reporting
thread for every measured thread just to repeatedly fulfil this same basic
pattern. But I can't think of anything better, so if you also can't, then r=me.

---

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/layout/layout_task.rs, line 311 [r1] (raw file):}
A naming nit: I've carefully used just reporter rather than memory_reporter in identifiers because memory is redundant -- there aren't any other kinds of reporters -- and just makes things more verbose. (This applies to lots of new identifiers added by this change.)

---

^{components/profile_traits/mem.rs, line 53 [r1] (raw file):}
This comment isn't quite right now that Reporter is a struct rather than a trait. I would like to suggest what it should now read but my brain is hurting. Want to have a crack yourself? :)

---

^{components/profile_traits/mem.rs, line 68 [r1] (raw file):}
I'd move this above pub struct Reporter.

---

Comments from the review on Reviewable.io

[jdm]

The reason for the threads right now is that it's impossible to select over in-process channels and IPC channels simultaneously. That means that we always need at least one thread to watch for IPC messages and route them to the proper place, much like Gecko has an IPC thread.

[pcwalton]

I should compose a better response to your comment after I've fully digested it, @nnethercote, but note that in the future we may be able to select over an fixed number of IPC channels (but only IPC channels, not a mixture of in-process and IPC). This is definitely doable on Unix and Windows (via select and WFMO respectively, as long as there are fewer than 64 channels you're selecting over), and maybe on Mac OS X—I'll have to investigate.

This may not help very much, though, as most tasks have to select over a combination of in-process and IPC channels.

[pcwalton]

I wonder if a workable solution might be to have a generic, per-process IPC-channel-to-sender router that ipc-channel manages stored in a global. We could expose a function that allows users of IPC channels to dynamically register "adapters" with it to allow bridging of IpcSender<T> to a Sender<T>.

[metajack]

Where does the 64 channel limitation come from? select() has the ability to handle thousands of fds, though it is limited.

[pcwalton]

WFMO has a limitation of 64 on W

[pcwalton]

WFMO has a limitation of 64 on Windows. You can't use select over named pipes there.

[pcwalton]

The future is now. I've upgraded ipc-channel to multiplex all routing communications traffic into a single thread, eliminating the necessity of spawning helper threads.

r? @nnethercote

[pcwalton]

or r? @jdm, either one

[nnethercote]

This looks good except you haven't addressed my three small comments from my previous review.

[bors-servo]

☔ The latest upstream changes (presumably #6572) made this pull request unmergeable. Please resolve the merge conflicts.

[pcwalton]

Rebased and updated per comments.

[nnethercote]

Reviewed 6 of 6 files at r2.
Review status: 5 of 18 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

^{components/profile_traits/mem.rs, line 59 [r3] (raw file):}
Nit: "encapsulate"

---

Comments from the review on Reviewable.io

[nnethercote]

r=me

[pcwalton]

@bors-servo: r=nnethercote

[bors-servo]

📌 Commit 2d285fc has been approved by nnethercote

[bors-servo]

⌛ Testing commit 2d285fc with merge ea31f16...

[bors-servo]

💔 Test failed - mac3

[bors-servo]

💔 Test failed - linux2

[jdm]

/dom/ranges/Range-insertNode.html
---------------------------------
TIMEOUT [Parent]
/dom/ranges/Range-mutations.html
--------------------------------
TIMEOUT [Parent]
/dom/ranges/Range-surroundContents.html
---------------------------------------
TIMEOUT [Parent]

?

[pcwalton]

@bors-servo: r=jdm

[bors-servo]

📌 Commit 4820d3c has been approved by jdm

[bors-servo]

⌛ Testing commit 4820d3c with merge a59a844...

[bors-servo]

💔 Test failed - linux2

[pcwalton]

We have two WPT failures here. One is the image onload stuff and another is a timeout that turned into a crash. My theory for the latter is that it is a case of e10s perturbing the timing causing a preexisting crash to manifest sooner. I will try to verify that locally. For the former, we know that these tests never really worked right to begin with and depending on the timing of image vs document onload will succeed or fail. I will first try to adjust the expectations and if that continues to fail I will ignore the tests. OK, @jdm?

[pcwalton]

Oh, and if my theory for the former is correct, then I will adjust the test expectations and/or ignore as appropriate.

[mbrubeck]

@bors-servo retry

[bors-servo]

🔒 Merge conflict

[bors-servo]

☔ The latest upstream changes (presumably #6705) made this pull request unmergeable. Please resolve the merge conflicts.

[pcwalton]

@bors-servo: r=jdm

[bors-servo]

📌 Commit f10c076 has been approved by jdm

[bors-servo]

⌛ Testing commit f10c076 with merge f778e0e...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3