Consider moving away from unmaintained gojay

[imsodin]

Gojay has not seen any activity in a long time, and one of it's dependencies isn't available at the given source path anymore, causing build failures:
francoispqt/gojay#128
syncthing/syncthing#8256

Looking at a recent comparison (https://medium.com/geekculture/my-golang-json-evaluation-20a9ca6ef79c) it looks like the standard encoding/json library is pretty decent for encoding (and there's only encoding in the usecase here, right?). Otherwise https://github.com/goccy/go-json or https://github.com/json-iterator/go look like good candidates.

[dmanlfc]

@marten-seemann can you update accordingly?

go: github.com/lucas-clemente/quic-go@v0.25.0 requires
	github.com/francoispqt/gojay@v1.2.13 requires
	cloud.google.com/go@v0.37.0 requires
	go.opencensus.io@v0.18.0 requires
	git.apache.org/thrift.git@v0.0.0-20180902110319-2566ecd5d999: invalid version: git fetch -f origin refs/heads/*:refs/heads/* refs/tags/*:refs/tags/* in /x86_64/host/usr/share/go-path/pkg/mod/cache/vcs/f6f367fb9d5656665ab3c2c1c3b2a44c2ffc79f6b70612f5864d7ceedcc7b9ea: exit status 128:
	fatal: repository 'https://git.apache.org/thrift.git/' not found

[marten-seemann]

What is there to update? We're already using the most recent gojay version.
Wasn't the Go module proxy intended to prevent this from happening?

[imsodin]

I don't think there's anything to update. And I don't understand under what circumstances that build issue is happening (doesn't for me). Maybe there's some build modes where said proxy isn't used? Anyway my request is more about dropping an unmaintained library (gojay), that also adds unnecessary dependencies - which as a nice side-effect will also fix this build error.

[dmanlfc]

@marten-seemann the thrift source site is outdated

[marten-seemann]

@imsodin, sorry, I was only replying to @dmanlfc yesterday.

Looking at a recent comparison (https://medium.com/geekculture/my-golang-json-evaluation-20a9ca6ef79c) it looks like the standard encoding/json library is pretty decent for encoding (and there's only encoding in the usecase here, right?).

You're right, the only thing we care about is decoding JSON. It's used for qlog, where we're logging (when enabled) every single frame in every single packet received (plus a bunch of other internal events). It's super valuable for debugging QUIC transfers, especially when combined with the awesome qvis.
That's a massive amount of data we're encoding, especially during high-bandwidth transfers. That's the reason why we picked gojay: it's really, really fast. Especially allocation-wise, and we're in a regime where GC pressure matters a lot.

I'd be very happy to move to encoding/json, but last time I benchmarked it, it was really slow (unfortunately, I don't have that benchmark lying around any more). Unfortunately, I don't currently have the time to tackle this transition.
@imsodin, would you be willing to help us out here? Encoding a packet_received event with a small number of frames (STREAM and ACK) would probably be the most interesting benchmark to look at.

[marten-seemann]

To add more details why encoding/json is out of the question: It would cause extra allocations (and GC pressure) for every single frame.

The frames passed to qlog are defined in the logging package: https://github.com/lucas-clemente/quic-go/blob/master/logging/frame.go. Most of them are just aliases for the internal frame types used by quic-go. qlog then serializes these frames without any allocations, for example here: https://github.com/lucas-clemente/quic-go/blob/332473668a992ce372c54c353b2bdc24344cf46e/qlog/frame.go#L133-L136

Now one might argue that one could define the JSON serialization of frames in the wire package (it would be a slight layer violation, but we could probably live with that, as qlog is being standardized by the IETF). However, this fails due to the insufficiencies of encoding/json. As you can see from the example above, the token needs to be wrapped in its on JSON object.

Otherwise https://github.com/goccy/go-json or https://github.com/json-iterator/go look like good candidates.

Note that this also immediately excludes these two candidates, as they try to emulate the interface of encoding/json.

[BRUHItsABunny]

Could perhaps use something more like:
https://github.com/bytedance/sonic
If performance is a concern.

[marten-seemann]

Could perhaps use something more like:
https://github.com/bytedance/sonic
If performance is a concern.

Also doesn't allow serialization without allocations. Really, moving away from gojay is only an option if we can preserve the performance properties. If not, I don't see any problem staying with gojay, even if it's unmaintained (it outputs correct JSON really fast, and that's the main thing that matters).

[imsodin]

My main motivation were the build failures caused by transitive dependencies. With module graph pruning since go1.17 this isn't a problem anymore, as the problematic dependency isn't actually used by quic-go thus since 1.17 go doesn't try to build it anymore. Now having benchmarks for this to check performance would still be good, e.g. in case gojay breaks somehow in the future, however it's a no longer an important concern for me so I don't plan to work on it in the near future.

[brotherdust]

gojay has a dependency on Apache Thrift. Apache's git repo is now on GH, so the url for the dependency is out of date and won't build.

[marten-seemann]

Can’t reproduce. Builds on my machine, and builds on CI.

[marten-seemann]

https://github.com/mailru/easyjson might be an alternative. It works with code generation (and generates quite a lot of LOC), but it is fast!

Here's a benchmark:

❯ go test -run afdsasd -bench . -benchmem
goos: darwin
goarch: arm64
pkg: github.com/quic-go/quic-go/qlog
BenchmarkPacketHeaderGoJay-10       	 5663672	       203.7 ns/op	      80 B/op	       1 allocs/op
BenchmarkPacketHeaderEasyJSON-10    	15633277	        78.40 ns/op	     107 B/op	       0 allocs/op

Code:

var h = packetHeader{
	PacketType:   getPacketTypeFromEncryptionLevel(protocol.EncryptionHandshake),
	PacketNumber: 1337,
	Version:      protocol.Version1,
}

func BenchmarkPacketHeaderGoJay(b *testing.B) {
	buf := bytes.NewBuffer(make([]byte, 0, 1000))
	enc := gojay.NewEncoder(buf)
	for i := 0; i < b.N; i++ {
		if err := enc.Encode(h); err != nil {
			b.Fatal(err)
		}
		buf.Reset()
	}
}

func BenchmarkPacketHeaderEasyJSON(b *testing.B) {
	buf := buffer.Buffer{Buf: make([]byte, 0, 1000)}
	w := &jwriter.Writer{Buffer: buf}
	for i := 0; i < b.N; i++ {
		h.MarshalEasyJSON(w)
		buf.DumpTo(io.Discard)
	}
}

One thing we'd need to figure out is how to rewrite custom marshaling logic such that the encoding output doesn't change. For example, we can't use "omitempty" for the ConnectionID type, as that's a struct. One option would be to use pointers, but that comes with a performance cost as well (which is probably way smaller than an additional alloc though).

[marten-seemann]

One more option we could look into: https://github.com/bytedance/sonic