Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585293002:
Reducing calling to SmoothingFilter in Fec Controller. (Closed)
|
Created:
4 years ago by minyue-webrtc Modified:
3 years, 11 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReducing calling to SmoothingFilter in Fec Controller.
BUG=webrtc:6303
Review-Url: https://codereview.webrtc.org/2585293002
Cr-Commit-Position: refs/heads/master@{#15806}
Committed: https://chromium.googlesource.com/external/webrtc/+/e1c2d9b0a8c25d95401fe9535f411f2ce08b5e02
Patch Set 1 #
Total comments: 7
Messages
Total messages: 18 (7 generated)
Description was changed from ========== Reducing calling to SmoothingFilter in Fec Controller. BUG=webrtc:6303 ========== to ========== Reducing calling to SmoothingFilter in Fec Controller. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
Hi Michael, Please take a look at this small CL.
lgtm
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Hi Henrik, Please also take a look!
lgtm with optional comment. https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; I think you can pass an Optional<float> by value. https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:93: const rtc::Optional<float>& packet_loss) const; And here.
https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; On 2016/12/22 10:42:39, hlundin-OOOtoJan9 wrote: > I think you can pass an Optional<float> by value. I am not sure about the efficiency between ref and copy when comes to Optional. I'd like to learn about it. I can commit it as it is now but would like to discuss with you about it and possibly do an update in a following patch.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1482846222820290, "parent_rev":
"6d3c57300b8399355e054565f285719e888bd452", "commit_rev":
"e1c2d9b0a8c25d95401fe9535f411f2ce08b5e02"}
Message was sent while issue was closed.
Description was changed from ========== Reducing calling to SmoothingFilter in Fec Controller. BUG=webrtc:6303 ========== to ========== Reducing calling to SmoothingFilter in Fec Controller. BUG=webrtc:6303 Review-Url: https://codereview.webrtc.org/2585293002 Cr-Commit-Position: refs/heads/master@{#15806} Committed: https://chromium.googlesource.com/external/webrtc/+/e1c2d9b0a8c25d95401fe9535... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/e1c2d9b0a8c25d95401fe9535...
Message was sent while issue was closed.
https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; On 2016/12/27 13:43:33, minyue-webrtc(OOOtoJan16) wrote: > On 2016/12/22 10:42:39, hlundin-OOOtoJan9 wrote: > > I think you can pass an Optional<float> by value. > > I am not sure about the efficiency between ref and copy when comes to Optional. > I'd like to learn about it. > > I can commit it as it is now but would like to discuss with you about it and > possibly do an update in a following patch. I think the argument from ArrayView applies here as well: https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/array_.... That is, the Optional type is "tiny", making it trivial to copy, and thus not worth to bother with pointers or references. But I usually call on kwiberg@ to keep me honest in these cases...
Message was sent while issue was closed.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; On 2017/01/09 14:46:16, hlundin-webrtc wrote: > On 2016/12/27 13:43:33, minyue-webrtc(OOOtoJan16) wrote: > > On 2016/12/22 10:42:39, hlundin-OOOtoJan9 wrote: > > > I think you can pass an Optional<float> by value. > > > > I am not sure about the efficiency between ref and copy when comes to > Optional. > > I'd like to learn about it. > > > > I can commit it as it is now but would like to discuss with you about it and > > possibly do an update in a following patch. > > I think the argument from ArrayView applies here as well: > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/array_.... > > That is, the Optional type is "tiny", making it trivial to copy, and thus not > worth to bother with pointers or references. But I usually call on kwiberg@ to > keep me honest in these cases... I'm pretty sure Henrik is right (and if I were writing this code, I would've just passed it by value). But I'll see if I can whip up a small test to verify this... (By the way, for non-tiny types, passing a const T* to the function is usually the optimal choice. It's just like a const reference, except that it can be null...)
Message was sent while issue was closed.
https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; On 2017/01/09 14:55:19, kwiberg-webrtc wrote: > On 2017/01/09 14:46:16, hlundin-webrtc wrote: > > On 2016/12/27 13:43:33, minyue-webrtc(OOOtoJan16) wrote: > > > On 2016/12/22 10:42:39, hlundin-OOOtoJan9 wrote: > > > > I think you can pass an Optional<float> by value. > > > > > > I am not sure about the efficiency between ref and copy when comes to > > Optional. > > > I'd like to learn about it. > > > > > > I can commit it as it is now but would like to discuss with you about it and > > > possibly do an update in a following patch. > > > > I think the argument from ArrayView applies here as well: > > > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/array_.... > > > > That is, the Optional type is "tiny", making it trivial to copy, and thus not > > worth to bother with pointers or references. But I usually call on kwiberg@ to > > keep me honest in these cases... > > I'm pretty sure Henrik is right (and if I were writing this code, I would've > just passed it by value). But I'll see if I can whip up a small test to verify > this... > > (By the way, for non-tiny types, passing a const T* to the function is usually > the optimal choice. It's just like a const reference, except that it can be > null...) So, I did the experiment, and it seems like even for something as simple as Optional<float>, the compiler won't use registers for argument passing. (Presumably, this is because it's a union and not a struct under the hood. Or something.) So for any type T, large or small, 1. Argument type const Optional<T>& is slightly better than const T* if the caller already has an Optional<T>. (It wins by a few instructions, including one conditional branch.) 2. Argument type const T* is much better than const Optional<T>& if the caller doesn't already have an Optional<T>. (It wins by the cost of constructing an Optional<T>, which includes a conditional branch and making a copy of a T.) 3. Argument type Optional<T> is always at least as bad as the other two options, since it has to make an extra copy of the Optional, which costs a conditional branch and making a copy of a T. So, recommendation: Never use an Optional<T> argument. Almost always use a const T* argument, except when you want to optimize hard to save a few instructions for the case where the caller already has an Optional<T> (in which case you should use const Optional<T>&). Moral of the story: Note how my original intuition was to go with the alternative that turned out to be the worst of the three. https://goo.gl/JWVCxC
Message was sent while issue was closed.
https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2585293002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:91: const rtc::Optional<float>& packet_loss) const; On 2017/01/10 02:19:28, kwiberg-webrtc wrote: > On 2017/01/09 14:55:19, kwiberg-webrtc wrote: > > On 2017/01/09 14:46:16, hlundin-webrtc wrote: > > > On 2016/12/27 13:43:33, minyue-webrtc(OOOtoJan16) wrote: > > > > On 2016/12/22 10:42:39, hlundin-OOOtoJan9 wrote: > > > > > I think you can pass an Optional<float> by value. > > > > > > > > I am not sure about the efficiency between ref and copy when comes to > > > Optional. > > > > I'd like to learn about it. > > > > > > > > I can commit it as it is now but would like to discuss with you about it > and > > > > possibly do an update in a following patch. > > > > > > I think the argument from ArrayView applies here as well: > > > > > > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/array_.... > > > > > > That is, the Optional type is "tiny", making it trivial to copy, and thus > not > > > worth to bother with pointers or references. But I usually call on kwiberg@ > to > > > keep me honest in these cases... > > > > I'm pretty sure Henrik is right (and if I were writing this code, I would've > > just passed it by value). But I'll see if I can whip up a small test to verify > > this... > > > > (By the way, for non-tiny types, passing a const T* to the function is usually > > the optimal choice. It's just like a const reference, except that it can be > > null...) > > So, I did the experiment, and it seems like even for something as simple as > Optional<float>, the compiler won't use registers for argument passing. > (Presumably, this is because it's a union and not a struct under the hood. Or > something.) So for any type T, large or small, > > 1. Argument type const Optional<T>& is slightly better than const T* if the > caller already has an Optional<T>. (It wins by a few instructions, including one > conditional branch.) > > 2. Argument type const T* is much better than const Optional<T>& if the caller > doesn't already have an Optional<T>. (It wins by the cost of constructing an > Optional<T>, which includes a conditional branch and making a copy of a T.) > > 3. Argument type Optional<T> is always at least as bad as the other two > options, since it has to make an extra copy of the Optional, which costs a > conditional branch and making a copy of a T. > > So, recommendation: Never use an Optional<T> argument. Almost always use a const > T* argument, except when you want to optimize hard to save a few instructions > for the case where the caller already has an Optional<T> (in which case you > should use const Optional<T>&). > > Moral of the story: Note how my original intuition was to go with the > alternative that turned out to be the worst of the three. https://goo.gl/JWVCxC Wow! Interesting findings. I stand corrected. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
