Remove custom binary-conversion functions

[joelegasse]

I didn't like the look of one of the utility functions I came across, so I went on a mission to purge its evil ways from our codebase. It was hiding allocations...

During the purge, I came across a few places where we were encoding things in a non-optimal manner, so I cleaned them up some in an attempt to reduce allocations and copying of data.

One of the cleanups was in the TSM WAL code, which probably caused the corrupt file seen in #5455. The old code can be perused to see more detail, but the short version is that a WAL entry with lots of long strings would get truncated, but on a per-key basis. That meant that one key would be shorted space, and the next key would be written in space that should be occupied by the length-prefixed strings... nasty edge case. The fix there was to traverse the entire entry, and all of it's values, and do one large allocation of the exact size required.

I was concerned about the performance impact, although I was optimistic it would improve. Here are my local benchmarks of the changes (only the interesting/significant ones):

benchmark                                    old ns/op      new ns/op      delta
BenchmarkWALSegmentWriter                    1528724        786889         -48.53%
BenchmarkIndirectIndex_UnmarshalBinary       3719743        2847992        -23.44%

benchmark                           old allocs     new allocs     delta
BenchmarkWALSegmentWriter           15019          5010           -66.64%
BenchmarkQueueAppend                11             9              -18.18%

benchmark                           old bytes     new bytes     delta
BenchmarkWALSegmentWriter           496923        224383        -54.85%
BenchmarkQueueAppend                109           94            -13.76%

[jwilder]

This looks odd? What's this for?

[joelegasse]

Further down, in (*point).MarshalBinary(), we binary-encode the time.Time objects. And rather than guessing, or making assumptions that the encoded size will never change, I added this to figure out the encoded size on startup, then use that to allocate the required space later.

[joelegasse]

I could marshal the time first and use the len() of the resulting data to allocate the final slice, though.

[jwilder]

I think that would be better since the time has to be marshal anyways.

[jwilder]

I think this swapped tmin and tmax from how they were previously encoded.

[joelegasse]

Nope. tmin is in buf[0:8] and the old value (tmax followed by block) is in buf[8:]. Then at line 499 (new number), you can see that the key is buf[:8] (the old tmin), and the value is buf[8:] (the old value)

[jwilder]

I see.

[joelegasse]

@jwilder Is this acceptable? Each entry is encoded into the same buffer, and then written one-at-a-time to the passed-in io.Writer. The method also now adheres to io.WriterTo instead of clashing method names with io.Writer

[jwilder]

I think that would work. AppendTo doesn't feel like the right name, but I can't think of a better one.

[joelegasse]

I've seen AppendX used in both math/big and strconv, but for formatted string output. I'm open to changing the name to something more appropriate, though

[jwilder]

👍