Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670643007:
Make AudioVector::operator[] inline and modify index calculation (Closed)
|
Created:
3 years, 10 months ago by hlundin-webrtc Modified:
3 years, 10 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, peah-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation.
BUG=webrtc:7159
Review-Url: https://codereview.webrtc.org/2670643007
Cr-Commit-Position: refs/heads/master@{#16627}
Committed: https://chromium.googlesource.com/external/webrtc/+/280eb224e2e0630ec94a6072777d58354357ee67
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rewrite index calculation #
Total comments: 4
Patch Set 3 : Review comments #
Messages
Total messages: 28 (11 generated)
henrik.lundin@webrtc.org changed reviewers: + solenberg@webrtc.org
PTAL
https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:114: return array_[(begin_index_ + index) % capacity_]; Integer division is still surprisingly expensive on some platforms. Are we certain it is needed here? Can we make the capacity a power of two and use bitwise AND instead?
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
Seeing as this is an attempt at optimization: have you run any benchmarks? https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:113: inline const int16_t& operator[](size_t index) const { The "inline" keyword isn't necessary---functions defined (as opposed to declared) in a class definition are inline by default. https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:114: return array_[(begin_index_ + index) % capacity_]; On 2017/02/02 19:29:55, the sun wrote: > Integer division is still surprisingly expensive on some platforms. Are we > certain it is needed here? Can we make the capacity a power of two and use > bitwise AND instead? Alternatively, can we be sure that 0 <= begin_index_ + index < 2 * capacity_? Conditionally subtracting capacity_ is probably cheaper than modulus.
On 2017/02/07 14:04:15, kwiberg-webrtc wrote: > Seeing as this is an attempt at optimization: have you run any benchmarks? I ran neteq_speed_test, but failed to see significant change. I will probably drop this CL for now. > > https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... > File webrtc/modules/audio_coding/neteq/audio_vector.h (right): > > https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... > webrtc/modules/audio_coding/neteq/audio_vector.h:113: inline const int16_t& > operator[](size_t index) const { > The "inline" keyword isn't necessary---functions defined (as opposed to > declared) in a class definition are inline by default. > > https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... > webrtc/modules/audio_coding/neteq/audio_vector.h:114: return > array_[(begin_index_ + index) % capacity_]; > On 2017/02/02 19:29:55, the sun wrote: > > Integer division is still surprisingly expensive on some platforms. Are we > > certain it is needed here? Can we make the capacity a power of two and use > > bitwise AND instead? > > Alternatively, can we be sure that 0 <= begin_index_ + index < 2 * capacity_? > Conditionally subtracting capacity_ is probably cheaper than modulus.
Description was changed from ========== Make AudioVector::operator[] inline ========== to ========== Make AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation. ==========
I took a stab at getting rid of the modulo operator. To profile the work, I used the AudioVectorTest.SubscriptOperator unittest. First, I modified the test code to use an array of length 10000000 instead of 10 (audio_vector_unittest.cc, line 36). Then I ran the test with the following (approximate) results. Baseline: 253 ms Patch set 1: 246 ms Patch set 2: 58 ms (Setting up the test took approximately 10 ms, as estimated by setting up the test but only writing and reading one element from the AudioVector under test.) https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:114: return array_[(begin_index_ + index) % capacity_]; On 2017/02/07 14:04:15, kwiberg-webrtc wrote: > On 2017/02/02 19:29:55, the sun wrote: > > Integer division is still surprisingly expensive on some platforms. Are we > > certain it is needed here? Can we make the capacity a power of two and use > > bitwise AND instead? > > Alternatively, can we be sure that 0 <= begin_index_ + index < 2 * capacity_? > Conditionally subtracting capacity_ is probably cheaper than modulus. I did something along those lines.
Good speedup! https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_vector.h:114: inline const int16_t& operator[](size_t index) const { You should also DCHECK that begin_index_ + index doesn't overflow: RTC_DCHECK_GE(begin_index_ + index, index); https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_vector.h:126: RTC_DCHECK_LT(ix, capacity_); Break out the index calculation to a separate function? You could make it private static inline, I think...
On 2017/02/13 14:21:13, hlundin-webrtc wrote: > I took a stab at getting rid of the modulo operator. > > To profile the work, I used the AudioVectorTest.SubscriptOperator unittest. > First, I modified the test code to use an array of length 10000000 instead of 10 > (audio_vector_unittest.cc, line 36). Then I ran the test with the following > (approximate) results. > > Baseline: 253 ms > Patch set 1: 246 ms > Patch set 2: 58 ms > > (Setting up the test took approximately 10 ms, as estimated by setting up the > test but only writing and reading one element from the AudioVector under test.) > > https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... > File webrtc/modules/audio_coding/neteq/audio_vector.h (right): > > https://codereview.webrtc.org/2670643007/diff/1/webrtc/modules/audio_coding/n... > webrtc/modules/audio_coding/neteq/audio_vector.h:114: return > array_[(begin_index_ + index) % capacity_]; > On 2017/02/07 14:04:15, kwiberg-webrtc wrote: > > On 2017/02/02 19:29:55, the sun wrote: > > > Integer division is still surprisingly expensive on some platforms. Are we > > > certain it is needed here? Can we make the capacity a power of two and use > > > bitwise AND instead? > > > > Alternatively, can we be sure that 0 <= begin_index_ + index < 2 * capacity_? > > Conditionally subtracting capacity_ is probably cheaper than modulus. > > I did something along those lines. Nice! Still a lot of computation *per sample* though. Granted, that is necessary for a generic operator[] impl, it would be interesting to see what a stab at the AudioMultiVector::ReadInterleavedFromIndex() would yield (we should be able to split that up into max two loops).
New patch set uploaded. I could not see any change in performance versus the previous one. PTAL. https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_vector.h:114: inline const int16_t& operator[](size_t index) const { On 2017/02/13 14:44:08, kwiberg-webrtc wrote: > You should also DCHECK that begin_index_ + index doesn't overflow: > > RTC_DCHECK_GE(begin_index_ + index, index); Done. https://codereview.webrtc.org/2670643007/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/audio_vector.h:126: RTC_DCHECK_LT(ix, capacity_); On 2017/02/13 14:44:08, kwiberg-webrtc wrote: > Break out the index calculation to a separate function? You could make it > private static inline, I think... Done.
lgtm for this change
lgtm
On 2017/02/14 12:59:31, kwiberg-webrtc wrote: > lgtm Thanks. FYI: The difference between the old and new is illustrated here: https://godbolt.org/g/WQjOph We remove one div operation, but add three other (presumably cheaper) operations.
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13458)
Description was changed from ========== Make AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation. ========== to ========== Make AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation. BUG=webrtc:7159 ==========
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/21150)
The CQ bit was checked by henrik.lundin@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": 40001, "attempt_start_ts": 1487155104002620,
"parent_rev": "a7b682aa18cad1db43bf8cd99a9ce9118760e80d", "commit_rev":
"280eb224e2e0630ec94a6072777d58354357ee67"}
Message was sent while issue was closed.
Description was changed from ========== Make AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation. BUG=webrtc:7159 ========== to ========== Make AudioVector::operator[] inline and modify the index calculation to avoid the modulo operation. BUG=webrtc:7159 Review-Url: https://codereview.webrtc.org/2670643007 Cr-Commit-Position: refs/heads/master@{#16627} Committed: https://chromium.googlesource.com/external/webrtc/+/280eb224e2e0630ec94a60727... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/280eb224e2e0630ec94a60727... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
