1167175 - Decouple WebRTC's LoadMonitor from RTC, move it to toolkit.

Status: NEW

(Toolkit :: General, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Looking at the code, we come back to a frequent problem: we're polling the system for information at regular intervals. I haven't tried tracing the polling interval back to its value, but it's potentially battery-unfriendly. That's probably not a problem for WebRTC, but that can be a problem for PerfMonitoring and possibly other consumers.

I believe that the right thing to do is to expose as a public API the code used for accessing System/Process load, rather than the entire LoadMonitor. This will let us tie polling to other activities that take place when we already know that the thread is awake – either the instant we perform polling for add-on monitoring or a when we enter/exit the event loop.

gcp, pkerr, does this sound ok?

Comment 2

[Paul Kerr [:pkerr]]

I think it would be lead to a clean separation of the details: how to get the data from each operation system and the caller/consumer. The only tricky part is that you load estimate needs to take time into account when not running on a periodic callback.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

So I'll try and move GetProcessLoad() and GetSystemLoad() to nsIPerformanceStatsService.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I have a WIP on moving UpdateProcessLoad()/UpdateSystemLoad() to toolkit, but 1/ that code is growing a bit too complex for my tastes; 2/ I'm starting to wonder whether this is the right thing to do.

I wonder if an API with the following primitives would not be better:
- acquire/release monitor;
- get latest value (without lock);
- get next value (also without lock).

With a single monitoring thread shared by the system, which runs iff at least one client has currently acquired the monitor. This would save resources, and the thread may be used by:
- WebRTC;
- the add-on monitor;
- the jank monitor;
- the unresponsive webpage monitor.

What do you think, Paul?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

I think you have the clearest view on a solution that satisfies all consumers fwiw. From the WebRTC side the important thing is that it can get a timely notification if the actual load changes. From its perspective battery doesn't factor in (it's maxing the CPU) and it does want continuous monitoring.

From your description it sounds like you want LoadMonitor clients to tell it whether they want opportunistic polling (battery friendly) or a maximum interval and adopt accordingly. The actual functions for retrieving the load could be pushed in a separate API even lower, sure.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: bz://1167175/Yoric

/r/9349 - Bug 1167175 - LoadMonitor, v2

Pull down this commit:

hg pull -r f1affbf998689c8c4e1ab949da414c24f6736458 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[David Teller [:Yoric] - still alive but not very active]

I have pushed to MozReview a possible replacement API for LoadMonitor. I believe that this API would satisfy the needs of PerfMonitoring. Would it also be sufficient for WebRTC?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

https://reviewboard.mozilla.org/r/9349/#review8057

::: dom/media/systemservices/LoadMonitor.h:86
(Diff revision 1)
> +     * thread, they may return NaN. Otherwise, a value in [.0, 1.0].

NaN sounds like a potential footgun.

Can you implement LoadManager in terms of this?

To me the fact that the clients don't signal the load granulairty they need makes it seem this can't achieve what was commented in this bug earlier.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

We could certainly implement the following without too many difficulties:

Acquire(float maxGranularity)

Would that be sufficient?

Comment 10

[Gian-Carlo Pascutto [:gcp]]

minGranularity sounds more logical :-)

LoadManager is currently the only consumer of LoadMonitor, so as long as you can keep that working your LoadMonitor changes should be fine for WebRTC.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Ok. I'll experiment with this and I'll try and find something better than NaN.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Looks like the LoadManager can be reimplemented easily on top of this LoadMonitor v2.

Comment 13

[Paul Kerr [:pkerr]]

Comment on attachment 8610507 [details]
MozReview Request: bz://1167175/Yoric

https://reviewboard.mozilla.org/r/9347/#review8187

::: dom/media/systemservices/LoadMonitor.h:98
(Diff revision 1)
> +    explicit InternalLoadMonitor(int aLoadUpdateInterval);

How is the aLoadUpdateInterval selected? Can the user of LoadMonitor select a minimum update interval for a callback registration?

Also, can the user select the minimum change in a load value that will trigger a callback notification?

Comment 14

[Paul Kerr [:pkerr]]

comment via review board, see above

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://reviewboard.mozilla.org/r/9347/#review9325

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8610507 [details]
MozReview Request: bz://1167175/Yoric

Sorry but I find reviewboard completely unusable and I have no idea how to close this.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1167175 - Implement a CPU load monitor in toolkit;r=mossop

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8620346 [details]
MozReview Request: Bug 1167175 - Implement a CPU load monitor in toolkit;r=mossop

Bug 1167175 - Implement a CPU load monitor in toolkit;r=mossop

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8620346 [details]
MozReview Request: Bug 1167175 - Implement a CPU load monitor in toolkit;r=mossop

Here's a first draft (not entirely tested yet).
For all non-Windows platforms, this largely a port of the code in LoadMonitor.cpp. For Windows, I used `GetSystemTimes` instead of a Process Counter, because it felt much simpler.

Comment 21

[Paul Kerr [:pkerr]]

https://reviewboard.mozilla.org/r/9349/#review12521

::: toolkit/components/perfmonitoring/nsSystemLoad.cpp:138
(Diff revision 2)
> +  bool success = GetSystemTimes(&idleTime, &kernelTime, &userTime);

As long as the GetSystemTimes API call closely tracks the system load, in both value and response time, as show by the Windows performance monitor utility, then there should be no problem with changing the load calculation method for windows platforms. Using the system.diagnotic interface to the process counter was intended to work in the the same manner as the windows diagnostic tools and tracked the output of those tools very well.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

By the way, do you know if the code you have been using is affected by the OS swapping to disk? I'm interested in [de]activating stuff conditionally based on whether the OS is responsive, and I'd like to include this in the definition of being not responsive.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Based on general experience, you're more likely to see the load drop if the OS is swapping to disk, as processing gets blocked on disk IO.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

That's what I feared.
So how do you cope with swapping (memory pressure?) in WebRTC.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Not? It's not like it's very relevant?

Comment 26

[Gian-Carlo Pascutto [:gcp]]

To clarify, we care about CPU usage because video and audio encoders have high CPU complexity (and could easily take 100% of the CPU in the quest for better quality, if that was desirable). Memory requirements are much more modest.

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Well, if I understand correctly, you use the LoadMonitor to decrease quality when you cannot cope with them because of system load. But swapping will also prevent the process from getting CPU time. which will cause the same kind of issues, no?

Comment 28

[Gian-Carlo Pascutto [:gcp]]

>swapping will also prevent the process from getting CPU time

No? Whatever needs to be swapped in won't be able to run at all but that shouldn't stop the (fairly small) core loops in the encoders (which are on separate threads) from pegging the CPU if possible.

A scenario where the WebRTC code is starved for CPU time because the DSP code in the encoders is swapped out is not one you need to consider, because before you get to that point everything else in the browser has already become completely unusable.

Comment 29

[Paul Kerr [:pkerr]]

I agree with what gcp has posted on this subject.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Fair enough. The code attached should work, but I'm putting this on the backburner, because it looks like system load is not the information I'm looking for after all.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

I've gone for an entire strategy change, so I'm dropping this bug. If someone wants to pick it, feel free.