Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700633003:
Further optimization of AudioVector::operator[] (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. |
DescriptionFurther optimization of AudioVector::operator[]
This is a follow-up to https://codereview.webrtc.org/2670643007/. That
CL provided significant improvement to Mac, Linux and ARM-based
platforms, but failed to improve the performance for Windows. The
problem is that the MSVC compiler did not produce branch-free code for
that fix. This new change produces the same result for non-Windows
platforms, as well as introduces branch-free code for Windows.
H/t to kwiberg@ for providing the solution.
BUG=webrtc:7159
Review-Url: https://codereview.webrtc.org/2700633003
Cr-Commit-Position: refs/heads/master@{#16649}
Committed: https://chromium.googlesource.com/external/webrtc/+/751589899b753d8ca56ac6e2ca2742596520cc8e
Patch Set 1 #
Total comments: 4
Messages
Total messages: 13 (6 generated)
Description was changed from ========== Further optimization of AudioVector::operator[] This is a follow-up to https://codereview.webrtc.org/2670643007/. That CL provided significant improvement to Mac, Linux and ARM-based platforms, but failed to improve the performance for Windows. The problem is that the MSVC compiler did not produce branch-free code for that fix. This new change produces the same result for non-Windows platforms, as well as introduces branch-free code for Windows. BUG=webrtc:7159 ========== to ========== Further optimization of AudioVector::operator[] This is a follow-up to https://codereview.webrtc.org/2670643007/. That CL provided significant improvement to Mac, Linux and ARM-based platforms, but failed to improve the performance for Windows. The problem is that the MSVC compiler did not produce branch-free code for that fix. This new change produces the same result for non-Windows platforms, as well as introduces branch-free code for Windows. H/t to kwiberg@ for providing the solution. BUG=webrtc:7159 ==========
henrik.lundin@webrtc.org changed reviewers: + kwiberg@webrtc.org
PTAL
lgtm!
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": 1, "attempt_start_ts": 1487255365226640, "parent_rev":
"3ebabf1c2983c99bfea9fe47373920d23dd99f55", "commit_rev":
"751589899b753d8ca56ac6e2ca2742596520cc8e"}
Message was sent while issue was closed.
Description was changed from ========== Further optimization of AudioVector::operator[] This is a follow-up to https://codereview.webrtc.org/2670643007/. That CL provided significant improvement to Mac, Linux and ARM-based platforms, but failed to improve the performance for Windows. The problem is that the MSVC compiler did not produce branch-free code for that fix. This new change produces the same result for non-Windows platforms, as well as introduces branch-free code for Windows. H/t to kwiberg@ for providing the solution. BUG=webrtc:7159 ========== to ========== Further optimization of AudioVector::operator[] This is a follow-up to https://codereview.webrtc.org/2670643007/. That CL provided significant improvement to Mac, Linux and ARM-based platforms, but failed to improve the performance for Windows. The problem is that the MSVC compiler did not produce branch-free code for that fix. This new change produces the same result for non-Windows platforms, as well as introduces branch-free code for Windows. H/t to kwiberg@ for providing the solution. BUG=webrtc:7159 Review-Url: https://codereview.webrtc.org/2700633003 Cr-Commit-Position: refs/heads/master@{#16649} Committed: https://chromium.googlesource.com/external/webrtc/+/751589899b753d8ca56ac6e2c... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/external/webrtc/+/751589899b753d8ca56ac6e2c...
Message was sent while issue was closed.
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
Message was sent while issue was closed.
lgtm, % documentation of supported input range. https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:127: static inline size_t WrapIndex(size_t index, This is going to work correctly only when index and begin_index are < capacity (unlike an implementation using % which would support a much wider range). I think that deserves a comment and/or DCHECKs to back it up. https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:132: begin_index + index - (begin_index + index >= capacity ? capacity : 0); I'm guess the compiler can recognize (begin_index + index) as a common subexpession, but I think the code would be more readable if assigning it to a variable. Like size_t ix = begin_index + index; RTC_DCHECK_GE(ix, index); if (ix >= capacity) ix -= capacity; Or some variation of the last line to get branch free code from relevant compilers. It ought to be compiled to the sequence move, sub, cmov, unless the compiler is more clever than I ;-)
Message was sent while issue was closed.
Good suggestions, but the CL already landed, so it's up to whoever likes them enough to do the work... https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_vector.h (right): https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:127: static inline size_t WrapIndex(size_t index, On 2017/02/17 08:28:26, nisse-webrtc wrote: > This is going to work correctly only when index and begin_index are < capacity > (unlike an implementation using % which would support a much wider range). I > think that deserves a comment and/or DCHECKs to back it up. We already DCHECK that ix got a reasonable value below, but I guess that if index and begin_index both had unreasonable values we could fail to detect that. So I guess adding the two DCHECKs you suggest would be good. https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_vector.h:132: begin_index + index - (begin_index + index >= capacity ? capacity : 0); On 2017/02/17 08:28:25, nisse-webrtc wrote: > I'm guess the compiler can recognize (begin_index + index) as a common > subexpession, but I think the code would be more readable if assigning it to a > variable. Like > > size_t ix = begin_index + index; > RTC_DCHECK_GE(ix, index); > if (ix >= capacity) ix -= capacity; > > Or some variation of the last line to get branch free code from relevant > compilers. It ought to be compiled to the sequence move, sub, cmov, unless the > compiler is more clever than I ;-) Yes, I agree that is more readable. I don't like mutating variables, but in this case it actually is better. (All the relevant compilers were able to eliminate the common subexpression in the old code, however, so this change would be purely for the humans.)
Message was sent while issue was closed.
On 2017/02/17 09:43:23, kwiberg-webrtc wrote: > Good suggestions, but the CL already landed, so it's up to whoever likes them > enough to do the work... > > https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... > File webrtc/modules/audio_coding/neteq/audio_vector.h (right): > > https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... > webrtc/modules/audio_coding/neteq/audio_vector.h:127: static inline size_t > WrapIndex(size_t index, > On 2017/02/17 08:28:26, nisse-webrtc wrote: > > This is going to work correctly only when index and begin_index are < capacity > > (unlike an implementation using % which would support a much wider range). I > > think that deserves a comment and/or DCHECKs to back it up. > > We already DCHECK that ix got a reasonable value below, but I guess that if > index and begin_index both had unreasonable values we could fail to detect that. > So I guess adding the two DCHECKs you suggest would be good. > > https://codereview.webrtc.org/2700633003/diff/1/webrtc/modules/audio_coding/n... > webrtc/modules/audio_coding/neteq/audio_vector.h:132: begin_index + index - > (begin_index + index >= capacity ? capacity : 0); > On 2017/02/17 08:28:25, nisse-webrtc wrote: > > I'm guess the compiler can recognize (begin_index + index) as a common > > subexpession, but I think the code would be more readable if assigning it to a > > variable. Like > > > > size_t ix = begin_index + index; > > RTC_DCHECK_GE(ix, index); > > if (ix >= capacity) ix -= capacity; > > > > Or some variation of the last line to get branch free code from relevant > > compilers. It ought to be compiled to the sequence move, sub, cmov, unless the > > compiler is more clever than I ;-) > > Yes, I agree that is more readable. I don't like mutating variables, but in this > case it actually is better. > > (All the relevant compilers were able to eliminate the common subexpression in > the old code, however, so this change would be purely for the humans.) Suggestions are incorporated in https://codereview.webrtc.org/2706263002/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
