Buffer bounds checks slow (writeUInt8 with noAssert = false)

[joliss]

The write family of functions on Buffer objects (buf.writeUInt8 etc.) have a lot of overhead when noAssert isn't set to true -- much more than I would normally expect from a bounds check.

Here are some timings I got for filling a 100MB buffer (Node 7.5.0 on 64-bit Linux):

Assigning buf[i], buf[i+1]: 392.496ms
buf.writeUInt16LE, noAssert: 342.778ms
buf.writeUInt16LE, without noAssert: 9072.283ms

Assigning buf[i]: 288.415ms
buf.writeUInt8, noAssert: 312.455ms
buf.writeUInt8, without noAssert: 8725.956ms

I had a lot of timing variation, so I don't think the indexed-vs-writeUInt8 differences are meaningful. This might be because we're down to about 10 CPU cycles per iteration. My bug report is about the unexpected order-of-magnitude difference when noAssert is not enabled.

Benchmark code:

let count = 100000000
let buf1, buf2

function timeAssign16() {
  let buf1
  buf1 = Buffer.allocUnsafe(count * 2)
  console.time('Assigning buf[i], buf[i+1]')
  for (let i = 0; i < count; i++) {
    let uint16 = i % 2**16
    buf1[i*2] = uint16 % 256
    buf1[i*2 + 1] = (uint16 >> 8) % 256
  }
  console.timeEnd('Assigning buf[i], buf[i+1]')
  return buf1
}

function timeWrite16() {
  let buf2
  buf2 = Buffer.allocUnsafe(count * 2)
  console.time('buf.writeUInt16LE, noAssert')
  for (let i = 0; i < count; i++) {
    buf2.writeUInt16LE(i % 2**16, i * 2, true)
  }
  console.timeEnd('buf.writeUInt16LE, noAssert')
  return buf2
}

function timeWrite16Checked() {
  let buf2
  buf2 = Buffer.allocUnsafe(count * 2)
  console.time('buf.writeUInt16LE, without noAssert')
  for (let i = 0; i < count; i++) {
    buf2.writeUInt16LE(i % 2**16, i * 2)
  }
  console.timeEnd('buf.writeUInt16LE, without noAssert')
  return buf2
}

buf1 = timeAssign16()
buf2 = timeWrite16()
buf2 = timeWrite16Checked()
if (buf1.compare(buf2) !== 0) throw 'error'

console.log()

function timeAssign8() {
  let buf1
  buf1 = Buffer.allocUnsafe(count)
  console.time('Assigning buf[i]')
  for (let i = 0; i < count; i++) {
    buf1[i] = i % 256
  }
  console.timeEnd('Assigning buf[i]')
  return buf1
}

function timeWrite8() {
  let buf2
  buf2 = Buffer.allocUnsafe(count)
  console.time('buf.writeUInt8, noAssert')
  for (let i = 0; i < count; i++) {
    buf2.writeUInt8(i % 256, i, true)
  }
  console.timeEnd('buf.writeUInt8, noAssert')
  return buf2
}

function timeWrite8Checked() {
  let buf2
  buf2 = Buffer.allocUnsafe(count)
  console.time('buf.writeUInt8, without noAssert')
  for (let i = 0; i < count; i++) {
    buf2.writeUInt8(i % 256, i)
  }
  console.timeEnd('buf.writeUInt8, without noAssert')
  return buf2
}

buf1 = timeAssign8()
buf2 = timeWrite8()
buf2 = timeWrite8Checked()
if (buf1.compare(buf2) !== 0) throw 'error'

Related: #11244 ("Unclear if Buffer buf[i] is bounds-checked"). If buf[i] is indeed bounds-checked, then surely we should be able to achieve the same performance with noAssert = false?

[mscdex]

Pull requests are always welcome, but since brackets do not throw on an invalid index, there would have to be explicit range checking (maybe even for performance reasons?).

[seishun]

I wonder if anyone relies on the exceptions thrown by checkInt. If Buffer were to be designed now, we wouldn't introduce the range checking at all since there is no undefined behavior now - assigning out-of-bounds is just no-op. Unfortunately there seems to be no deprecation path that would let us get rid of it.

[seishun]

I decided to run some benchmarks, some experiments and some more benchmarks on my 64-bit Windows 10 machine.

Node's own benchmarks confirm that noAssert=false causes a major performance degradation:

The checkInt function that's called when noAssert is false has three checks. I tried removing each one by one to see if a specific check is responsible for the slowdown. Removing the value check or the range check individually didn't make much difference. Removing them both (leaving just the Uint8Array check) didn't either:

Removing the Uint8Array check (and leaving the other two), however, made a huge difference:

Without the Uint8Array check, noAssert=false still causes a significant slowdown, but at least it's not tenfold anymore:

I have no idea why exactly this check was necessary in the first place (it was added in one huge commit), so I propose the following course of action:

1. Submit a PR removing the Uint8Array check. It shouldn't even be semver-major.
2. Discuss switching noAssert to being true by default. Checks that introduce a performance degradation and are in most cases unnecessary should be opt-in.

/cc @nodejs/collaborators

[seishun]

@joliss Now that #11324 has landed, the difference shouldn't be so drastic anymore. Could you look at benchmark results on master and let us know what you think?

[TimothyGu]

# v7.7.1
$ node buftest.js 
Assigning buf[i], buf[i+1]: 442.359ms
buf.writeUInt16LE, noAssert: 404.190ms
buf.writeUInt16LE, without noAssert: 11098.851ms

Assigning buf[i]: 253.650ms
buf.writeUInt8, noAssert: 288.419ms
buf.writeUInt8, without noAssert: 11238.774ms
# master
$ ./node buftest.js 
Assigning buf[i], buf[i+1]: 541.285ms
buf.writeUInt16LE, noAssert: 416.756ms
buf.writeUInt16LE, without noAssert: 1000.491ms

Assigning buf[i]: 300.127ms
buf.writeUInt8, noAssert: 286.881ms
buf.writeUInt8, without noAssert: 830.433ms

The slow-down is not nearly as drastic. Closing.