commit 3f1de2c6e8535c6101c94b45c465e384c6bcc826
author Mark Hansen <markhansen@google.com> Wed Sep 11 16:49:54 2024 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Sep 11 16:58:01 2024 -0700
tree 4071260705dd84a578de0a46f23cb05c6f2a62cf
parent f861f12841ad80f96073de6dc458368ee51d6f63

CodedOutputStream: Avoid updating position to go beyond end of array.

This has twofold goals:
1. Correctness: if position overruns the array, checking space left may return a negative number. I'm not sure how bad that is, but let's avoid it.
2. Performance. This generates more optimal assembly code which can combine bounds checks, particularly on Android (I haven't looked at the generated assembly on the server JVM; it's possible the server JVM can already performance this hoist).

The `position` field is stored on the object, so Android ART generates assembly codes for `this.position++` like "load, add, store":

```
       ldr w3, [x1, #12]
       add w4, w3, #0x1 (1)
       str w4, [x1, #12]
```

There can be a lot of these loads/stores executed each step of a loop (e.g. writeFixed64NoTag updates position 8 times, and varint encoding could do it even more). It's faster if we can hoist these so we load once at the start of the function, and store once at the end of the function. This also has the nice benefit that it won't store if we've thrown an exception.

See before/after in Compiler Explorer: https://godbolt.org/z/bWWYqsxK4. I'm not an assembly expert, but it seems clear that the increment instructions like `add w4, w0, #0x1 (1)` are no longer always surrounded by loads and stores in the new version.

And in Compiler Explorer, you also see `bufferFixed64NoTag` has reduced from 98 lines of assembly to 57 lines of assembly in the hoisted version. This is because we don't need to re-check the array bounds each time we reload `position`. I imagine this also makes any other method with a fixed number of increments like `writeFixed32NoTag` faster too.

PiperOrigin-RevId: 673588324