Make background hang monitor optional

[jdm]

In performance-sensitive situations like immersive mode on the hololens, having the background hang monitor thread running shows up in profiles. We should make it a configuration option at engine startup, and put the channels to communicate with the thread inside Option values so we don't have unnecessary channel traffic.

[gterzian]

One way, perhaps the easiest, to do this, would:

1. Set a flag using servo_config.
2. In the BHM:

Here:

servo/components/background_hang_monitor/background_hang_monitor.rs

Line 26 in 7ac4f9e

pub fn init(

You could read the flag, and if we don't want to actually start a worker:

1. Don't start one,
2. return an implementation of BackgroundHangMonitorRegister that basically does nothing, where register_component would itself return an implementation of BackgroundHangMonitor that would do nothing.

put the channels to communicate with the thread inside Option values so we don't have unnecessary channel traffic

If the above solution is still too much, in the sense that components will still allocate and drop messages when they use the empty BackgroundHangMonitor, then we could indeed actually make changes to the client code and avoid any operation from happening.

[CYBAI]

Maybe we can use the features of cargo and init the hang monitor like this 👀?

servo/components/servo/lib.rs

Lines 58 to 64 in 7ac4f9e

	#[cfg(feature = "webdriver")]
	fn webdriver(port: u16, constellation: Sender<ConstellationMsg>) {
	webdriver_server::start_server(port, constellation);
	}
	#[cfg(not(feature = "webdriver"))]
	fn webdriver(_port: u16, _constellation: Sender<ConstellationMsg>) {}

[gterzian]

Yes that could be a good idea, how would that work exactly with the BHM being a sub-crate and with the workspace setup of the repo as a whole? I'm looking around online and if you already know how it would be done, please describe it here!

[CYBAI]

@gterzian I'm not familiar with how BHM works but I imagine maybe we can do it like

For example, for the following piece of code, we will initiate the BHM if the feature is specified.

let (background_monitor_register, sampler_chan) = if cfg!(feature = "background_hang_monitor") {
    let (sampling_profiler_control, sampling_profiler_port) =
        ipc::channel().expect("ipc channel failure");

    (
        Some(HangMonitorRegister::init(
            background_hang_monitor_sender.clone(),
            sampling_profiler_port,
        )),
        vec![sampling_profiler_control],
    )
} else {
    (None, vec![])
};

servo/components/constellation/constellation.rs

Lines 846 to 862 in 393b676

	// If we are in multiprocess mode,
	// a dedicated per-process hang monitor will be initialized later inside the content process.
	// See run_content_process in servo/lib.rs
	let (background_monitor_register, sampler_chan) = if opts::multiprocess() {
	(None, vec![])
	} else {
	let (sampling_profiler_control, sampling_profiler_port) =
	ipc::channel().expect("ipc channel failure");
	(
	Some(HangMonitorRegister::init(
	background_hang_monitor_sender.clone(),
	sampling_profiler_port,
	)),
	vec![sampling_profiler_control],
	)
	};

Maybe @jdm will have better plan though, WDYT 👀?

[CYBAI]

☝️ In this solution, I mean, if we don't specify --features="background_hang_monitor, then we don't start a BHM so that we might be able to avoid using it when we want?

[gterzian]

Looks like a good idea. I think what I proposed with different implementations of the current traits is actually too complicated.

[jdm]

As a developer, it would be ideal if this were a runtime rather than compile-time choice. My preference would be using Options around channels like we do with the devtools server.

[kunalmohan]

@jdm could you point out the devtools server implementation that you referred above. It would be helpful if I take a look at it first.

[jdm]

This is where the devtools server is started, and the channel to communicate with it is stored in an Option. Similarly this is client code that will communicate with the devtools server if it exists.

[kunalmohan]

If I wrap the value in background_hang_monitor_receiver inside Option, how do I handle it inside select macro?

servo/components/constellation/constellation.rs

Lines 1416 to 1418 in 87d28b9

	recv(self.background_hang_monitor_receiver) -> msg => {
	msg.expect("Unexpected BHM channel panic in constellation").map(Request::BackgroundHangMonitor)
	}

[gterzian]

I think the example "Optionally add a receive operation to select! using never" could be useful for this, see https://docs.rs/crossbeam-channel/0.4.0/crossbeam_channel/macro.select.html