Make the BERT protocol encoding aware

[tenderlove]

This PR makes the BERT protocol aware of Ruby encodings. I have updated the Encoder and the Decoder as follows:

Decoder

The decoder now looks for a magic header and changes parsing styles based on the header it finds. If the header is MAGIC, then it knows that the encoder didn't add any encoding information and uses the old decoder. If the header is VERSION2, then it knows that the encoder added string encoding information and it adds the encoding information to the string.

Encoder

The encoder is split in to version 1 and version 2 encoding. It defaults to version 1, and version 2 is enabled with a feature flag. The reason I did this is so that we can deploy the updated bert gem to both the client and the server, then flip the feature flag on for the client, then flip it on for the server. That way we can upgrade without downtime.

Here is an IRB session to demonstrate the encoding support:

[aaron@TC bert (encoding)]$ irb -I lib:ext
irb(main):001:0> require 'bert'
=> true
irb(main):002:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語")).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):003:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語"))
=> "\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"
irb(main):004:0> BERT::Encode.version = :v2
=> :v2
irb(main):005:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語")).encoding
=> #<Encoding:UTF-8>
irb(main):006:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語"))
=> "日本語"
irb(main):007:0>

/cc @brianmario @tmm1 @charliesome @piki

[piki]

This is a good direction. Any measurements for how much of a performance hit we take with any sort of representative workload? This change essentially doubles the network, RAM, and CPU cost of short strings. How much difference does it make in a real RPC scenario?

If performance is at all an issue, we could indicate common encodings (I expect to see a lot of UTF-8) with something cheaper than a second String.

[brianmario]

If performance is at all an issue, we could indicate common encodings (I expect to see a lot of UTF-8) with something cheaper than a second String.

In mochilo we used genperf to create a perfect hash lookup table mapping Ruby encoding names to integer values. I know using the actual encoding id from ruby itself isn't deterministic (depends on load order or something?) but maybe we could do something similar to mochilo one-time and as Ruby adds new encodings we can add them to the end of the list as new ids?

That way we could use a single byte to represent the encoding and map it to/from the encoding name on the server and client at [de]serialization time? That is, if performance is a big issue here...

[tenderlove]

It looks like finding an encoding by name is just a hash lookup (given the encoding has been loaded once already), so I don't think we need to worry about the speed of finding the encoding object.

I agree that representing the encoding as an integer would reduce the payload size, but I don't know if it matters. Using MIME would require even more network chatter than this. If we could guarantee that all strings in one payload were of the same type, then we could really reduce data. But since strings in Ruby are allowed to have different encodings, I don't know if that's something we can / want to enforce.

[piki]

It's the number and collective size of allocations that concerns me. Grabbing a list of all the refs in a repository, or all the refs on a particular branch, could be hundreds of thousands of small strings. This change just doubles the serialization, deserialization, and allocation costs. An int would definitely be cheaper.

[tenderlove]

@piki do you know if payloads usually have homogenous encodings? If stuff is usually UTF-8, maybe we could only add extra encoding information if it's not UTF-8. If strings are usually UTF-8, then we'd only incur overhead for "special" strings.

[carlosmn]

I would expect 90% of the cases to be utf8 strings, since that's the encoding we use (assume) when we extract Git data via rugged, or to be raw binary like from spawning a child process.

Having a way to specify both of these via a single int should help a lot with the smaller strings (and would still allow us to specify a different encoding if it's neither of these).

[tenderlove]

Ok, I added two new types to the protocol. One type is for UTF-8 strings, and one type is for "other" encoded strings. Strings tagged as ASCII-8BIT will use the BIN type, so there's no change in protocol overhead for them. The only strings that have additional overhead now are strings that are tagged with some encoding other than UTF8, US-ASCII, and ASCII-8BIT.

Here's a demo of UTF8 strings:

[aaron@TC bert (encoding)]$ irb -I lib
irb(main):001:0> require 'bert'
=> true
irb(main):002:0> BERT::Encoder.encode("日本語").bytesize
=> 15
irb(main):003:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語")).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):004:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語"))
=> "\xE6\x97\xA5\xE6\x9C\xAC\xE8\xAA\x9E"
irb(main):005:0> BERT::Encode.version = :v2
=> :v2
irb(main):006:0> BERT::Encoder.encode("日本語").bytesize
=> 15
irb(main):007:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語")).encoding
=> #<Encoding:UTF-8>
irb(main):008:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語"))
=> "日本語"
irb(main):009:0>

Binary strings:

[aaron@TC bert (encoding)]$ irb -I lib -rbert
irb(main):001:0> BERT::Encoder.encode("日本語".b).bytesize
=> 15
irb(main):002:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語".b)).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):003:0> BERT::Encode.version = :v2
=> :v2
irb(main):004:0> BERT::Encoder.encode("日本語".b).bytesize
=> 15
irb(main):005:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語".b)).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):006:0>

US-ASCII:

[aaron@TC bert (encoding)]$ irb -I lib -rbert
irb(main):001:0> BERT::Encoder.encode("teelo".encode('US-ASCII')).bytesize
=> 11
irb(main):002:0> BERT::Decoder.decode(BERT::Encoder.encode("teelo".encode('US-ASCII'))).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):003:0> BERT::Encode.version = :v2
=> :v2
irb(main):004:0> BERT::Encoder.encode("teelo".encode('US-ASCII')).bytesize
=> 11
irb(main):005:0> BERT::Decoder.decode(BERT::Encoder.encode("teelo".encode('US-ASCII'))).encoding
=> #<Encoding:UTF-8>
irb(main):006:0>

And finally Shift_JIS:

[aaron@TC bert (encoding)]$ irb -I lib -rbert
irb(main):001:0> BERT::Encoder.encode("日本語".encode('Shift_JIS')).bytesize
=> 12
irb(main):002:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語".encode('Shift_JIS'))).encoding
=> #<Encoding:ASCII-8BIT>
irb(main):003:0> BERT::Encode.version = :v2
=> :v2
irb(main):004:0> BERT::Encoder.encode("日本語".encode('Shift_JIS')).bytesize
=> 26
irb(main):005:0> BERT::Decoder.decode(BERT::Encoder.encode("日本語".encode('Shift_JIS'))).encoding
=> #<Encoding:Shift_JIS>
irb(main):006:0>

Note that US-ASCII gets lifted to UTF8, but I think that doesn't matter, and the Shift_JIS string is a bit longer when encoded because it embeds the encoding name.

[brianmario]

Nice work!

Note that US-ASCII gets lifted to UTF8, but I think that doesn't matter

👍 to that

[piki]

I like it. 👍

[haileys]

Looks great ✨

[carlosmn]

IIUC this is the first time we're diverging from erlang's External Term Format. And as said format does have these values (for NEW_FUN_EXT and EXPORT_EXT resp.), I think a comment explaining that we knowingly diverge here for version 2 would be useful.

[piki]

Are there other (available) values we should use instead?

Adherence to erlang formats isn't important for GitHub's uses of BERT -- which all involve Ruby -- but I could imagine it being an issue if anyone else is still using BERT with erlang.

[carlosmn]

Assuming the online docs are up to date, numbers from 120 onwards are free, but they could probably add new types at any point.

We could reduce the divergence by using 107 (STRING_EXT) for utf8 strings whose length fits within 2 bytes and go with ENC_STRING if they're bigger, but we'd still re-use or hope we never find new types useful for ENC_STRING.

The types we're making unavailable aren't something that I think make sense to transfer between ruby and erlang, so I don't think anybody will miss them. But even so making an note of the rationale in the code would be useful.

[tenderlove]

I chose these values because they are used for the offset in the callbacks struct. Would adding a comment and removing the ERL prefix be enough?

[carlosmn]

The use in the offset makes it a bit trickier. How about we use ERLEXT_ and have a comment saying these are specific to our own v2? My concern here is coming back to the code and having to figure out how the C, ruby and spec match up so leaving a note would be polite :)

[tenderlove]

Done. I added a comment! :D