Further optimization of AudioVector::operator[]

webrtc/modules/audio_coding/neteq/audio_vector.h

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 106 matching lines @@
 
   inline int16_t& operator[](size_t index) {
     return array_[WrapIndex(index, begin_index_, capacity_)];
   }
 
  private:
   static const size_t kDefaultInitialSize = 10;
 
   // This method is used by the [] operators to calculate an index within the
   // capacity of the array, but without using the modulo operation (%).
   static inline size_t WrapIndex(size_t index,

[nisse-webrtc, 2017/02/17 08:28:26]

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.

[kwiberg-webrtc, 2017/02/17 09:43:23]

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.

                                  size_t begin_index,
                                  size_t capacity) {
     RTC_DCHECK_GE(begin_index + index, index);  // Check for overflow.
-    const size_t ix = begin_index + index >= capacity
-                          ? begin_index + index - capacity
-                          : begin_index + index;
+    const size_t ix =
+        begin_index + index - (begin_index + index >= capacity ? capacity : 0);

[nisse-webrtc, 2017/02/17 08:28:25]

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 ;-)

[kwiberg-webrtc, 2017/02/17 09:43:23]

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.)

     RTC_DCHECK_LT(ix, capacity);
     return ix;
   }
 
   void Reserve(size_t n);
 
   void InsertByPushBack(const int16_t* insert_this, size_t length,
                         size_t position);
 
   void InsertByPushFront(const int16_t* insert_this, size_t length,
@@ skipping 12 matching lines @@
   size_t begin_index_;
 
   // The index of the sample after the last sample in |array_|.
   size_t end_index_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(AudioVector);
 };
 
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_NETEQ_AUDIO_VECTOR_H_