Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender

Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's inefficient to perform string comparison whenever we send a packet.

BUG=None

Review-Url: https://codereview.webrtc.org/2637203002
Cr-Commit-Position: refs/heads/master@{#16291}
Committed: https://chromium.googlesource.com/external/webrtc/+/c3dfff3126e4756c573cc1bdb95b28c57d1a66d7

[elad.alon_webrtc.org, 2017-01-17 19:16:24]

Description was changed from

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender
==========

to

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's
inefficient to perform string comparison whenever we send a packet.
==========

[elad.alon_webrtc.org, 2017-01-17 19:16:24]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org,
sprang@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-01-17 19:17:21]

Could anyone give the way to edit the source behind FindFullName, so that I'll
make sure it is populated before the RTPSender ctor is called?

[elad.alon_webrtc.org, 2017-01-17 19:29:12]

On 2017/01/17 19:17:21, elad.alon wrote:
> Could anyone give the way to edit the source behind FindFullName, so that I'll
> make sure it is populated before the RTPSender ctor is called?

I see it's done from main() in the demo app. Is this the case for all relevant
applications?

[minyue-webrtc, 2017-01-18 08:14:38]

https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (left):

https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1259: if
(webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") ==
but it is called only once, isn't it?

[minyue-webrtc, 2017-01-18 08:15:51]

On 2017/01/17 19:29:12, elad.alon wrote:
> On 2017/01/17 19:17:21, elad.alon wrote:
> > Could anyone give the way to edit the source behind FindFullName, so that
I'll
> > make sure it is populated before the RTPSender ctor is called?
> 
> I see it's done from main() in the demo app. Is this the case for all relevant
> applications?

Field trial is meant to be set up on the app level.

[stefan-webrtc, 2017-01-18 08:41:40]

I believe this should be safe. Field trials should be set on the
PeerConnectionFactory before any PeerConnection has been created.

https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (left):

https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1259: if
(webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") ==
On 2017/01/18 08:14:38, minyue-webrtc(OOOtoJan17) wrote:
> but it is called only once, isn't it?

Once for every packet sent. Still a lot... :)

[stefan-webrtc, 2017-01-18 08:42:02]

lgtm

[minyue-webrtc, 2017-01-18 09:20:11]

On 2017/01/18 08:41:40, stefan-webrtc wrote:
> I believe this should be safe. Field trials should be set on the
> PeerConnectionFactory before any PeerConnection has been created.
> 
>
https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
> File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (left):
> 
>
https://codereview.webrtc.org/2637203002/diff/1/webrtc/modules/rtp_rtcp/sourc...
> webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1259: if
> (webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead") ==
> On 2017/01/18 08:14:38, minyue-webrtc(OOOtoJan17) wrote:
> > but it is called only once, isn't it?
> 
> Once for every packet sent. Still a lot... :)

OK!

[minyue-webrtc, 2017-01-18 09:20:23]

lgtm

[michaelt, 2017-01-18 09:45:48]

looks ok. As far that i know we have the same issue in transport feedback
adaptor. When we add the transport overhead. Should we do the same trick there
as well?

[stefan-webrtc, 2017-01-18 09:48:24]

On 2017/01/18 09:45:48, michaelt wrote:
> looks ok. As far that i know we have the same issue in transport feedback
> adaptor. When we add the transport overhead. Should we do the same trick there
> as well?

Yes, that would be good. Can you take a look?

[elad.alon_webrtc.org, 2017-01-18 10:01:19]

On 2017/01/18 09:48:24, stefan-webrtc wrote:
> On 2017/01/18 09:45:48, michaelt wrote:
> > looks ok. As far that i know we have the same issue in transport feedback
> > adaptor. When we add the transport overhead. Should we do the same trick
there
> > as well?
> 
> Yes, that would be good. Can you take a look?

Sure. I was thinking of adding a little mechanism to make sure that the function
is not called more than X times with the same string - this validation to only
be used in debug - to catch other instances of this problem. Do you guys think
it would be useful?

[michaelt, 2017-01-18 10:13:33]

i don't know if this a good idea. i guess we would run in to troubles in the
unit test. And it will hard to say how may calls will be too much. Since the
checks can be spread over the code like in "bwe with overhead"

[elad.alon_webrtc.org, 2017-01-18 10:33:07]

On 2017/01/18 10:13:33, michaelt wrote:
> i don't know if this a good idea. i guess we would run in to troubles in the
> unit test. And it will hard to say how may calls will be too much. Since the
> checks can be spread over the code like in "bwe with overhead"

My idea is to RTC_DCHECK(not_too_many_calls(str)), where not_too_many_calls() is
a function that writes to a local std::map<std::string, size_t>. In debug
builds, this would make sure it's not called too often. In release, it will have
no effect, and so incur no performance penalty.
We can get around it triggering in UT by either of the following:
* Setting the threshold for too many calls differently for different
applications.
* Setting the threshold high enough that it won't be triggered by UT, but would
be triggered by applications (every single RTP packet > UT tests).
* Having UT reset the counters.

[sprang_webrtc, 2017-01-18 10:35:34]

lgtm

[stefan-webrtc, 2017-01-18 10:35:42]

On 2017/01/18 10:33:07, elad.alon wrote:
> On 2017/01/18 10:13:33, michaelt wrote:
> > i don't know if this a good idea. i guess we would run in to troubles in the
> > unit test. And it will hard to say how may calls will be too much. Since the
> > checks can be spread over the code like in "bwe with overhead"
> 
> My idea is to RTC_DCHECK(not_too_many_calls(str)), where not_too_many_calls()
is
> a function that writes to a local std::map<std::string, size_t>. In debug
> builds, this would make sure it's not called too often. In release, it will
have
> no effect, and so incur no performance penalty.
> We can get around it triggering in UT by either of the following:
> * Setting the threshold for too many calls differently for different
> applications.
> * Setting the threshold high enough that it won't be triggered by UT, but
would
> be triggered by applications (every single RTP packet > UT tests).
> * Having UT reset the counters.

For me it's good enough to just manually ensure we call it in the constructors.
It should hopefully be gone in a not too distant future.

[elad.alon_webrtc.org, 2017-01-25 17:16:29]

I'll land this and push the other fixes for a separate review, then.

[elad.alon_webrtc.org, 2017-01-25 17:18:13]

The CQ bit was unchecked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-01-25 17:18:13]

The CQ bit was checked by elad.alon@webrtc.org

[commit-bot: I haz the power, 2017-01-25 17:18:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-25 17:22:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 17:22:55]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12675)

[elad.alon_webrtc.org, 2017-01-25 17:50:42]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-01-25 17:50:43]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2637203002/#ps20001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-01-25 17:50:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-25 17:55:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 17:55:40]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12677)

[elad.alon_webrtc.org, 2017-01-25 18:05:15]

The CQ bit was checked by elad.alon@webrtc.org

[commit-bot: I haz the power, 2017-01-25 18:05:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-25 18:10:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 18:10:09]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12679)

[elad.alon_webrtc.org, 2017-01-25 18:36:30]

The CQ bit was checked by elad.alon@webrtc.org

[elad.alon_webrtc.org, 2017-01-25 18:36:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
stefan@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2637203002/#ps40001 (title:
"Trying again...")

[commit-bot: I haz the power, 2017-01-25 18:36:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-25 18:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 18:42:08]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/12681)

[elad.alon_webrtc.org, 2017-01-26 10:30:55]

Description was changed from

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's
inefficient to perform string comparison whenever we send a packet.
==========

to

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's
inefficient to perform string comparison whenever we send a packet.

BUG=None
==========

[elad.alon_webrtc.org, 2017-01-26 10:31:15]

The CQ bit was checked by elad.alon@webrtc.org

[commit-bot: I haz the power, 2017-01-26 10:31:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-26 10:46:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485426675849060,
"parent_rev": "52cdd3bb305190d287db73ee29bc4370a5ce2788", "commit_rev":
"c3dfff3126e4756c573cc1bdb95b28c57d1a66d7"}

[commit-bot: I haz the power, 2017-01-26 10:46:59]

Description was changed from

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's
inefficient to perform string comparison whenever we send a packet.

BUG=None
==========

to

==========
Avoid multiple calls to webrtc::field_trial::FindFullName in RTPSender; it's
inefficient to perform string comparison whenever we send a packet.

BUG=None

Review-Url: https://codereview.webrtc.org/2637203002
Cr-Commit-Position: refs/heads/master@{#16291}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c3dfff3126e4756c573cc1bdb...
==========

[commit-bot: I haz the power, 2017-01-26 10:47:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/c3dfff3126e4756c573cc1bdb...