Closed Bug 1007775 Opened 5 years ago Closed 5 years ago

Port load/bitrate adaptation mechanism to all platforms

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: pkerr, Assigned: pkerr)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-][p=13])

Attachments

(2 files, 4 obsolete files)

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.
Attached patch Part 1: OS X port (obsolete) — Splinter Review
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.
Attached patch Part 2: Win32 port (obsolete) — Splinter Review
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.
Attachment #8419678 - Flags: review?(gpascutto)
Attachment #8419680 - Flags: review?(gpascutto)
Attachment #8419678 - Flags: review?(gpascutto) → review+
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().
Attachment #8419680 - Flags: review?(gpascutto) → review-
@@ +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?
Flags: needinfo?(gpascutto)
> ::: 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?
>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.
Flags: needinfo?(gpascutto)
Attached patch Part 2: Win32 port (obsolete) — Splinter Review
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.
Attachment #8419680 - Attachment is obsolete: true
Attachment #8421238 - Flags: review?(gpascutto)
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?
Attachment #8421238 - Flags: review?(gpascutto) → review+
Attachment #8419678 - Attachment is obsolete: true
Attachment #8421238 - Attachment is obsolete: true
Attachment #8423879 - Flags: review+
Attachment #8423880 - Flags: review+
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?
Attachment #8423880 - Attachment is obsolete: true
Attachment #8423913 - Flags: review+
Keywords: checkin-needed
Made a successful "all platforms" with unit tests try run.
(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
Attachment #8423879 - Flags: review+
Attachment #8423913 - Flags: review+
Depends on: 1013907
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)
Flags: needinfo?(pkerr)
It won't be needed until WebRTC activates/the LoadManager is started.
Flags: needinfo?(pkerr)
You need to log in before you can comment on or make changes to this bug.