Profiler for blocked IpcReceiver::recv()

[nakul02]

Implements feature #19223

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix Report time spent in synchronous IPC to the profiler #19223 (github issue number if applicable).

• There are tests for these changes

WIP.
@jdm - this is the new profiler : "Blocked at IPC Receive"
Should I dig through all the calls to ipc::channel and replace them with this profiled IpcReceiver?

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/screen.rs
• @fitzgen: components/profile/time.rs, components/script/dom/screen.rs, components/profile_traits/lib.rs, components/profile_traits/time.rs, components/profile_traits/Cargo.toml and 1 more
• @KiChjang: components/script/dom/screen.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[jdm]

Yes, replacing all the users would be really useful!

[nakul02]

@jdm - I've gone ahead and replaced calls to ipc_channel::ipc::channel to the newly introduced profile_traits::receiver::channel where calls were made to recv in the components/script/dom folder.
Do you want this done project wide for every call to ipc_channel::ipc::channel.
In most other places, there may not be a simple way to get a handle on the ProfilerChan instance.

[nakul02]

Ping @jdm

[jdm]

From my point of view, the more we use this API, the more confident we can be that we're actually noticing the biggest offenders. That being said, if you would prefer to merge the changes that you've made so far, we can also continue changing other channels in followup PRs.

[nakul02]

@jdm - if you have suggestions or guidelines for components that I should check for, I can go in and change those. Otherwise, for now, I'd prefer to merge this.
Does the name profiler_traits::receiver look ok? It is invoked everywhere as receiver::channel.

[jdm]

@nakul02 What if we used profiler_traits::ipc instead? I think this is a good set of channels to start with, and we can file follow up issues for other instances that make sense, because there are a lot of uses of channels where they are expected to spend a lot of time waiting in recv() and we don't want to report that.

[jdm]

Also, I bet we could write a test for this functionality in https://github.com/servo/servo/blob/master/tests/unit/profile/time.rs. Something like:

1. create a regular channel to mimic the time profiler
2. create a profiled channel
3. create a thread that waits for 2 seconds then sends a value with the sender
4. call recv() in the original thread
5. call recv() on the time profiler receiver, assert the its reports a realistic value for the time spent waiting on the first recv()
   Does that make sense?

[nakul02]

@jdm - I've added a test, but can't figure out why its failing for the life of me. This is the output from its failure:

---- time::channel_profiler_test stdout ----
	thread 'time::channel_profiler_test' panicked at 'called `Result::unwrap()` on an `Err` value: Io(Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" })', libcore/result.rs:916:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:68
             at libstd/sys_common/backtrace.rs:57
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:380
   3: std::panicking::default_hook
             at libstd/panicking.rs:390
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:576
   5: std::panicking::begin_panic
             at libstd/panicking.rs:537
   6: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:521
   7: rust_begin_unwind
             at libstd/panicking.rs:497
   8: core::panicking::panic_fmt
             at libcore/panicking.rs:71
   9: core::result::unwrap_failed
             at /checkout/src/libcore/macros.rs:23
  10: <core::result::Result<T, E>>::unwrap
             at /checkout/src/libcore/result.rs:782
  11: profile_tests::time::channel_profiler_test
             at tests/unit/profile/time.rs:59
  12: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1441
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:788
  13: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102


failures:
    time::channel_profiler_test

In this PR, there is a new enum type in the ProfilerMsg enum to be able to ask for a vector of recorded times, given information about a ProfilerBuckets. I've also Serialize-d and Deserialize-d the data that is sent through the channel (or so I hope), but none of it seems to be doing the trick.
Could you please take a look at the code from the last commit to see if anything pops out to you. If it doesn't any suggestion about a simpler way of testing this will help.

[jdm]

These changes look great!

[jdm]

No need to create a new thread here.

[jdm]

Hmm. Unfortunately, this is probably hitting a know bug in ipc-channel that we haven't been able to track down yet. Is there any change if you clone the channel before passing it as an argument?

[nakul02]

Ping @jdm

[jdm]

There are many more updates to Cargo.lock than this change requires. Did you run cargo update -a? Could you revert the changes to that file and run ./mach build again instead?

[jdm]

Apart from the Cargo.lock changes, this looks great!

[bors-servo]

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

[nakul02]

Thanks @jdm!
I deleted the Cargo.lock and ran mach build -d. That is what caused all those updates to Cargo.lock.
Have fixed that in this latest rebase and commit.

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit ac40d61 has been approved by jdm

[bors-servo]

⌛ Testing commit ac40d61 with merge 8660555...

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 7d4e2b1 has been approved by jdm

[nakul02]

@jdm - is there a way to just cause Taskcluster to run again?

[bors-servo]

⌛ Testing commit 7d4e2b1 with merge f467bdc...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing f467bdc to master...