Faster bulk numeric reads from BufferedIndexInput

[original-brownbear]

Reading ints/floats/longs one-by-one from a heap-byte-buffer, including doing our own bounds checks is not very efficient. We can use the ability to translate the buffer and read in bulk while taking turns with one-off reading/refilling instead.

[stefanvodita]

I left some nit-picky comments and some questions, but overall this looks like a good and useful change.

[stefanvodita]

Nit: Could we change the argument names to be consistent (dst, len)? I see the parent class, DataInput, has the same inconsistency.

[stefanvodita]

Nit: I found remaining to be a little confusing just because the buffer also calls to remaining(). What if we called it remainingDst to better differentiate from the buffer's remaining?

[stefanvodita]

I'm wondering why we're following a different pattern with these methods than with readBytes. If we rewrote readBytes using this model, would it just work?

  @Override
  public final void readBytes(byte[] dst, int offset, int len) throws IOException {
    int remaining = len;
    while (remaining > 0) {
      int cnt = Math.min(buffer.remaining(), remaining);
      buffer.get(dst, offset + len - remaining, cnt);
      remaining -= cnt;
      if (remaining > 0) {
        if (buffer.hasRemaining()) {
          dst[offset + len - remaining] = readByte();
          --remaining;
        } else {
          refill();
        }
      }
    }
  }

[original-brownbear]

I think it would come out to about that code if we inlined readBytes with useBuffer=true except that

        if (buffer.hasRemaining()) {
          dst[offset + len - remaining] = readByte();
          --remaining;
        }

is never entered, hasRemaining is just always false because we don't have anything like 1 one of 4 int bytes available.

-> the difference I think is just in that we have to deal with the alignment at the buffer boundary

[stefanvodita]

Should TestBufferedIndexInput have some basic tests for these new methods?

[original-brownbear]

++ good call, already had one bug that I only found when running benchmarks. Added a test for each of the 3 that should cover all the boundary/alignment/offset cases we can expect.

[jpountz]

This looks good to me. Can you add an entry to CHANGES.txt?

[jpountz]

nit: would it make more sense to compare the int bits rather than the float itself, to make sure +/-0 or the different representations of NaN are considered different?

[original-brownbear]

Right that's technically more exact I think :) Adjusted that and added changes.txt entry

[jpountz]

This is the section for the upcoming Lucene major (10.0), your change looks safe for backport, let's move it to section 9.8?

[jpountz]

I pushed a change

[original-brownbear]

Thanks Adrien!