Rate calculator optimization #538

vpalmisano · 2021-03-31T15:56:21Z

In the current implementation of RateCalculator class, the RemoveOldData has an high CPU usage because it should iterate over an array where each item index represents a time moment with a resolution of 1ms. As a consequence, in most cases we have a sparse array. This PR changes the calculator algorithm, assigning each sample to an array position in a contiguous way, avoiding large iterations when removing the expired elements. The unit test has been modified accordingly.

Test is needed!

no downsides
no bad corner cases
no bugs

Callgrind outputs:

v3:
with optimization:

worker/include/RTC/RateCalculator.hpp

worker/src/RTC/RateCalculator.cpp

penguinol · 2021-04-02T01:59:12Z

I've also tried to optimize rate calcautor by using deque，see penguinol@009f78b, but the performence improvement is not as good as i excepted.
The performence under -O3 is quite different from which under -O0.
In our test, the original rate calcautor cost about 5% cpu time under -O3.

vpalmisano · 2021-04-02T08:20:37Z

@penguinol right, I've run the same comparison with -O3:

v3: 1.45%
with my optimization: 0.67%

Anyway, RateCalculator::Update() is the mediasoup function with the greatest self CPU usage:
v3:

with my optimization:

ibc · 2021-04-02T10:49:56Z

Thanks for this. How to test this? and how to test that there are no downsides? are unit tests enough?

@jmillan can you please take a look to this?

vpalmisano · 2021-04-02T11:03:48Z

I have used this env variable to trace the process:
MEDIASOUP_USE_VALGRIND=true MEDIASOUP_VALGRIND_OPTIONS='--tool=callgrind --dump-instr=yes --simulate-cache=yes'

Looking at the source code, the RateCalculator class is used only to estimate the recvTransmission and sendTransmission to be reported into the transport statistics. If this is true, I see no potential critical issues.

ibc · 2021-04-02T11:25:25Z

Ok, let's merge next week (bank holidays here).

jmillan · 2021-04-13T09:31:35Z

We are currently using oldestTime and latestTime names. Same for indexes. This IMHO makes difficult readability.

I propose using: newestXXX & oldestXXX or firstXXX & lastXXX instead.

Personally I prefer newestXXX& oldestXXX. Being related to time, it makes more sense to me. What do you think @ibc, @vpalmisano?

jmillan · 2021-04-13T09:36:05Z

worker/include/RTC/RateCalculator.hpp

+		RateCalculator(
+		  size_t windowSize    = DefaultWindowSize,
+		  float scale          = DefaultBpsScale,
+		  uint16_t windowItems = DefaultWindowItems)


Is there any reason/case for windowItems value to be different than windowSize value?

I see no setter for it, and it takes the same value as windowSize in constructor, by default.

I wonder the very same.

The windowItems value depends on both the rate evaluation window period (windowSize) and the events frequency.
For instance, if the measured event frequency is 1 every 2ms, you can set windowSize=1000 and windowItems=500.

windowSize is only used to get the scale factor and to retrieve the new oldestTimeIndex whereas windowItems is the range (from 0 to windowItems) where the items are really stored in the buffer.

Having a windowSize bigger than the windowItems does not provide accurate results I would say. Ie:

windowSize = 1000 |-----------------------------------------------------| windowItems = 500 |------------------------|

We are calculating the rate for a time window of 1 second, and only considering the data of the last 1/2 second, which is not accurate.

Due to that rationale I think windowItems should be removed. Any comment on this @vpalmisano?

The idea is using different variables to represent the maximum window size expressed in seconds and in number of items, without mixing the two things.
The windowSize is expressed in seconds, while the windowItems is the maximum number of items that can be stored at the same time in the time window, so in your example you must take into account also the frequency of the data sample.
If you receive 1 sample per millisecond, you will fill the array with 500 items considering only 1/2 second of data.
Instead, if the data sample frequency is 1 every 2 milliseconds, a 500 items array will correspond to 1 second of data.
I can perform same tests evaluating if the window is filled or not when changing the windowItems value.

@jmillan right, the problem was not there before this PR. The drawback of the old approach is that it is using a number of items == window size in ms and in some cases this size could be equal to 2500 or 6000 packets.

Can we re-open this PR or should I open a new one with the latest fixes?

PRs cannot be reopened AFAIK so create a new one, please.

@notorca thanks for reporting the bug and comments.
Fixed now in: #547

Nice, I will test it soon.

@vpalmisano Works good for me.

ibc · 2021-04-13T10:59:44Z

Personally I prefer newestXXX& oldestXXX. Being related to time, it makes more sense to me.

Agreed

vpalmisano · 2021-04-13T11:10:26Z

We are currently using oldestTime and latestTime names. Same for indexes. This IMHO makes difficult readability.

I propose using: newestXXX & oldestXXX or firstXXX & lastXXX instead.

Personally I prefer newestXXX& oldestXXX. Being related to time, it makes more sense to me. What do you think @ibc, @vpalmisano?

Or maybe we can use oldestTime, latestTime and oldestTimeIndex, latestTimeIndex.

vpalmisano · 2021-04-13T11:19:09Z

For the same reason it will be better to rename windowSize -> windowDuration.

jmillan · 2021-04-13T15:21:45Z

Or maybe we can use oldestTime, latestTime and oldestTimeIndex, latestTimeIndex.

The thing is that oldest and latest are not antonyms. oldest and newest are, first and last are antonyms too. Being antonyms makes it easier to understand and relate each other.

jmillan · 2021-04-13T15:25:02Z

Wrapping up with your proposal @vpalmisano:

oldestTime, newestTime and oldestTimeIndex, newestTimeIndex would make it.

vpalmisano · 2021-04-14T15:15:16Z

Wrapping up with your proposal @vpalmisano:

oldestTime, newestTime and oldestTimeIndex, newestTimeIndex would make it.

Done with the latest commit.

…ano/mediasoup into optimize-rate-calculator

ibc · 2021-04-19T20:59:09Z

Are we ready to merge this? or is there something missing?

vpalmisano · 2021-04-20T07:47:36Z

I've replaced some variable names and added a warning log in the case the calculation buffer is full. I think there are no other changes to be done. @jmillan ?

worker/src/RTC/RateCalculator.cpp

jmillan · 2021-04-20T08:19:04Z

LGMT, thanks @vpalmisano!

Co-authored-by: José Luis Millán <[email protected]>

ibc · 2021-04-20T11:52:20Z

Merging!

ibc · 2021-04-20T11:55:10Z

Just a thing:

  CXX(target) /Users/ibc/src/v3-mediasoup/worker/out/Release/obj.target/mediasoup-worker-test/src/RTC/RtpObserver.o
../src/RTC/RateCalculator.cpp:33:6: warning: format specifies type 'unsigned long long' but the
      argument has type 'size_t' (aka 'unsigned long') [-Wformat]
                          this->windowSize,
                          ^~~~~~~~~~~~~~~~~
../include/Logger.hpp:435:22: note: expanded from macro 'MS_WARN_TAG'
        #define MS_WARN_TAG MS_WARN_TAG_STD
                            ^
../include/Logger.hpp:227:90: note: expanded from macro 'MS_WARN_TAG_STD'
  ..._MS_LOG_STR_DESC desc _MS_LOG_SEPARATOR_CHAR_STD, _MS_LOG_ARG, ##__VA_ARGS__); \
                      ~~~~                                            ^~~~~~~~~~~

ibc · 2021-04-20T11:57:14Z

fixing in master

MrSurana · 2021-04-29T10:07:04Z

Too many buffer full warnings after this commit.

2021-04-29T09:58:02.592Z mediasoup:WARN:Channel [pid:29676] RTC::RateCalculator::Update() | calculation buffer full, windowSize:2500 ms windowItems:1000

ibc · 2021-04-29T10:19:36Z

Too many buffer full warnings after this commit.

2021-04-29T09:58:02.592Z mediasoup:WARN:Channel [pid:29676] RTC::RateCalculator::Update() | calculation buffer full, windowSize:2500 ms windowItems:1000

#547

rate calculator optimization

16d89ad