1007775 - Port load/bitrate adaptation mechanism to all platforms

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Paul Kerr [:pkerr]]

Extend the platforms on which the load adaption mechanism introduced in bug 877954 will operate. The patch set from 877954 was limited to Android and Linux builds. This feature needs to be added to OS X, Win32 and FxOS.

Comment 1

[Paul Kerr [:pkerr]]

Attachment: Part 1: OS X port

Port of load adaptation feature to OS X. Primary change is the replacement of usage of the proc file system with call to the mach library to generate overall system load measurements.

Comment 2

[Paul Kerr [:pkerr]]

Attachment: Part 2: Win32 port

Port of load adaptation feature to Win32. Overall system load is queried from the pdh.dll performance counter. Process load is generated by tracking time spent in user and priviledge mode versus overall time count. For Win32, the number of processors is needed to generate a process load value.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8419680 [details] [diff] [review]
Part 2: Win32 port

Review of attachment 8419680 [details] [diff] [review]:
-----------------------------------------------------------------

There's some whitespace changes in the patch that just generate noise in the diff - don't do this on lines you're not actually changing.

::: content/media/webrtc/LoadManagerFactory.cpp
@@ +14,5 @@
>  LoadManager* LoadManagerBuild(void)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +#if defined(ANDROID) || defined(LINUX) || defined(XP_MACOSX) || defined(XP_WIN)

Are there tier1-supported platforms left we don't support here? If not all the conditionals can go by this point.

::: content/media/webrtc/LoadMonitor.cpp
@@ +161,5 @@
> +  WinProcMon():
> +      mQuery(0), mCounter(0) {};
> +  ~WinProcMon();
> +  nsresult Init();
> +  nsresult SystemLoad(float& load_percent);

Don't do this. It makes it not-obvious you're changing the value, see the Coding Style Guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +168,5 @@
> +  PDH_HCOUNTER mCounter;
> +};
> +
> +static LPCTSTR TotalCounterPath = _T("\\Processor(_Total)\\% Processor Time");
> +static const uint64_t FileTimestampTicksPerInterval = 10000000;

See remark below about the naming of this const.

@@ +222,5 @@
> +  load_percent = 0;
> +  // Update all counters associated with this query object.
> +  PDH_STATUS status = PdhCollectQueryData(mQuery);
> +
> +  if (status != ERROR_SUCCESS) {

Log the errors?

@@ +237,5 @@
> +
> +  if (ERROR_SUCCESS != status ||
> +      // There are multiple success return values.
> +      !IsSuccessSeverity(counter.CStatus)) {
> +    return NS_ERROR_FAILURE;

Same.

@@ +241,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // The result is a percent value, reduce to match expected scale.
> +  load_percent = (float)(counter.doubleValue / 100);

100.0f

@@ +285,5 @@
>    LoadStats mSystemLoad;
>    LoadStats mProcessLoad;
>    uint64_t mTicksPerInterval;
>    int mLoadUpdateInterval;
> +  int mNumProcessors;

Only needed on XP_WIN.

@@ +290,5 @@
>  };
>  
>  LoadInfo::LoadInfo(int aLoadUpdateInterval)
> +  : mLoadUpdateInterval(aLoadUpdateInterval),
> +    mNumProcessors(1)

Setting it to a constant here and then correcting it later for some platforms (but not others) is awkward. I'd remove this from here.

@@ +296,5 @@
>  #if defined(ANDROID) || defined(LINUX) || defined(XP_MACOSX)
>    mTicksPerInterval = (sysconf(_SC_CLK_TCK) * mLoadUpdateInterval) / 1000;
> +#elif defined(XP_WIN)
> +  mProcHandle = 0;
> +  mTicksPerInterval = FileTimestampTicksPerInterval * mLoadUpdateInterval;

The units (and hence the names) seem off here. The outcome of the multiplication would seem to be FileTimestampTicks, not TicksPerInterval.

One of the variables can't be named right.

@@ +305,5 @@
> +{
> +#ifdef XP_WIN
> +  SYSTEM_INFO sys_info;
> +  GetSystemInfo(&sys_info);
> +  mNumProcessors = sys_info.dwNumberOfProcessors;

aka PR_GetNumberOfProcessors()

@@ +308,5 @@
> +  GetSystemInfo(&sys_info);
> +  mNumProcessors = sys_info.dwNumberOfProcessors;
> +  mProcHandle = GetCurrentProcess();
> +  UpdateProcessLoad();
> +  return mSysMon.Init();

Why does the else clause do the two Update* calls? As far as I can tell mSysMon.Init() doesn't call the UpdateSystemLoad immediately, so either they're unnecessary or this side of the if is wrong.

@@ +447,5 @@
> +
> +  GetSystemTimeAsFileTime(&clk_time);
> +  total_times = (((uint64_t)clk_time.dwHighDateTime) << 32)
> +                + (uint64_t)clk_time.dwLowDateTime;
> +  bool ok = GetProcessTimes(mProcHandle, &clk_time, &clk_time, &sys_time, &user_time);

BOOL isn't actually the same as bool. GCC (if we'd use it for Win32) would give you a warning here because this requires the return value to be actually coaxed to a bool, even though you only care whether its == 0 or not. So assign to BOOL and check for == or != 0.

@@ +476,5 @@
>        mLoadNoiseCounter(0)
>    {
>      mLoadMonitor = loadMonitor;
>      mLoadInfo = new LoadInfo(mLoadUpdateInterval);
> +    mLoadInfo->Init();

Why make it return anything if you don't check the result? Could as well have put it in the constructor then, and done away with Init().

Comment 4

[Paul Kerr [:pkerr]]

@@ +296,5 @@
>  #if defined(ANDROID) || defined(LINUX) || defined(XP_MACOSX)
>    mTicksPerInterval = (sysconf(_SC_CLK_TCK) * mLoadUpdateInterval) / 1000;
> +#elif defined(XP_WIN)
> +  mProcHandle = 0;
> +  mTicksPerInterval = FileTimestampTicksPerInterval * mLoadUpdateInterval;

The units (and hence the names) seem off here. The outcome of the multiplication would seem to be FileTimestampTicks, not TicksPerInterval.

One of the variables can't be named right.

1) What is the definition of TicksPerInterval? I have been reading it as the number of clock units (ticks) of the clock used to return process state times (user, system, ...) in the sleep interval of the sampling thread.
2) Why is the value for the sysclk derived value divided by 1000?

Comment 5

[Paul Kerr [:pkerr]]

> ::: content/media/webrtc/LoadManagerFactory.cpp
> @@ +14,5 @@
> >  LoadManager* LoadManagerBuild(void)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +#if defined(ANDROID) || defined(LINUX) || defined(XP_MACOSX) || defined(XP_WIN)
> 
> Are there tier1-supported platforms left we don't support here? If not all
> the conditionals can go by this point.
> 
Where does B2G fall? Is it covered by the linux build?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>What is the definition of TicksPerInterval? I have been reading it as the number of clock units (ticks) of the clock used to return process state times (user, system, ...) in the sleep interval of the sampling thread.

That's right.

>2) Why is the value for the sysclk derived value divided by 1000?

sysclk isn't divided. It's mLoadUpdateInterval (which is stated in milliseconds) that needs to be divided.

I'm guessing FileTimestampTicksPerInterval is really FileTimestampTicksPerMillisecond?

>Where does B2G fall? Is it covered by the linux build?

B2G is ANDROID + MOZ_TOOLKIT_GONK or something.

Comment 7

[Paul Kerr [:pkerr]]

Attachment: Part 2: Win32 port

LoadManagerFactory.cpp and moz.build: dropped platform check conditionals since Android, Linux, OS X and Win32 platforms are now operable. LoadMonitor.cpp: 1) moved Win32 PDH setup and initialization code from the LoadInfoCollectRunner constructor to the LoadMonitor / LoadInfo Init chain with more error logging. 2) Fixed the units problem with the Win32 mTicksPerInterval calculation.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8421238 [details] [diff] [review]
Part 2: Win32 port

Review of attachment 8421238 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/LoadMonitor.cpp
@@ +468,5 @@
>  class LoadInfoCollectRunner : public nsRunnable
>  {
>  public:
>    LoadInfoCollectRunner(nsRefPtr<LoadMonitor> loadMonitor,
> +                        LoadInfo* loadInfo)

Document there's a transfer of ownership here. Maybe both of them can be RefPtrs, given the next remarks.

@@ +543,5 @@
>  LoadMonitor::Init(nsRefPtr<LoadMonitor> &self)
>  {
>    LOG(("Initializing LoadMonitor"));
>  
> +  LoadInfo* load_info = new LoadInfo(mLoadUpdateInterval);

You probably want a smart pointer here to ease the error handling, see below.

@@ +546,5 @@
>  
> +  LoadInfo* load_info = new LoadInfo(mLoadUpdateInterval);
> +  nsresult rv = load_info->Init();
> +
> +  if (rv != NS_OK) {

NS_FAILED

@@ +547,5 @@
> +  LoadInfo* load_info = new LoadInfo(mLoadUpdateInterval);
> +  nsresult rv = load_info->Init();
> +
> +  if (rv != NS_OK) {
> +    LOG(("LoadInfo::Init error"));

We should probably percolate the failure upwards here? Certainly the rest of the code isn't going to do much if this failed. (And don't leak memory if you do, see above)

@@ +550,5 @@
> +  if (rv != NS_OK) {
> +    LOG(("LoadInfo::Init error"));
> +  }
> +
> +#if defined(ANDROID) || defined(LINUX) || defined(XP_MACOSX) || defined(XP_WIN)

This can go now?

Comment 9

[Paul Kerr [:pkerr]]

Attachment: Part 1: OS X port of load adapation. Replace usage of proc fs for total system load with calls to mach library

tag with reviewer

Comment 10

[Paul Kerr [:pkerr]]

Attachment: Part 2: Win32 port of load adaptation. Get total system load from the PDH service. Calculate process load using user + privileged cpu ticks compared to system time ticks

clean up of RefPtr usage

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8423880 [details] [diff] [review]
Part 2: Win32 port of load adaptation. Get total system load from the PDH service. Calculate process load using user + privileged cpu ticks compared to system time ticks

Review of attachment 8423880 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/LoadMonitor.cpp
@@ +549,5 @@
>  
> +  RefPtr<LoadInfo> load_info = new LoadInfo();
> +  nsresult rv = load_info->Init(mLoadUpdateInterval);
> +
> +  if (rv != NS_OK) {

NS_FAILED(rv)

::: content/media/webrtc/moz.build
@@ +24,5 @@
>          'MediaEngineWebRTCAudio.cpp',
>          'MediaEngineWebRTCVideo.cpp',
>      ]
> +    if (CONFIG['OS_ARCH'] == 'Android' or CONFIG['OS_ARCH'] == 'Linux'
> +        or CONFIG['OS_ARCH'] == 'Darwin' or CONFIG['OS_ARCH'] == 'WINNT'):

Can't this go now?

Comment 12

[Paul Kerr [:pkerr]]

Attachment: Part 2: Win32 port of load adaptation. Get total system load from the PDH service. Calculate process load using user + privileged cpu ticks compared to system time ticks

incorporate final review feedback

Comment 13

[Paul Kerr [:pkerr]]

Made a successful "all platforms" with unit tests try run.

Comment 14

[Paul Kerr [:pkerr]]

https://tbpl.mozilla.org/?tree=Try&rev=342699377634

Comment 15

[Paul Kerr [:pkerr]]

(In reply to Paul Kerr [:pkerr] from comment #12)
> Created attachment 8423913 [details] [diff] [review]
> Part 2: Win32 port of load adaptation. Get total system load from the PDH
> service. Calculate process load using user + privileged cpu ticks compared
> to system time ticks
> 
> incorporate final review feedback

carrying forward r+=gcp

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f496c23c9a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3683786269

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f496c23c9a5
https://hg.mozilla.org/mozilla-central/rev/fa3683786269

Comment 18

[(Away)]

I happened to notice pdh.dll while poking around with depends.exe. Is that library really needed on every browser startup? Can it be delay-loaded? (Happy to open a bug for it if so)

Comment 19

[Gian-Carlo Pascutto [:gcp]]

It won't be needed until WebRTC activates/the LoadManager is started.