Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use jute generated structs #28

Open
nemith opened this issue Jun 9, 2020 · 7 comments
Open

Use jute generated structs #28

nemith opened this issue Jun 9, 2020 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@nemith
Copy link

nemith commented Jun 9, 2020

I spent some time writing https://github.com/go-zookeeper/jute which is nearly complete which can auto generate structures and how to serialize/deserialize them to the wire similar to Java which will illuminate the reflection and bring a bit more protection as well.

@pmazzini pmazzini self-assigned this Jun 10, 2020
@pmazzini
Copy link

This is interesting!

@jeffbean
Copy link

I'm leaning more now to not build and maintain something that supports what at this point is a dead IDL. I think the value of maintaining our structs is lower since ZK itself does not break any backwards compatibility.

Adding this is really going to be low ROI and falls into the boat of keeping simple may be best for the repo.

In place of that I think organizing, and better documenting the models we do maintain, as well as making some level of effort to follow and do some proactive additions when ZK major revs would be sufficient.

@nemith
Copy link
Author

nemith commented Apr 13, 2024

You are correct at the time this task was created i think it was less clear. It has been some years.

That being said most of the work as been done. Using jute is a matter of simple implementation. We didn't do it before as an intern at Facebook implemented a new zk library that used jute but that never went forward. It still exists here https://github.com/facebookarchive/zk. Facebook/Meta no longer use this API for ZK servers (and i think they moved away from zookeeper all together).

The win isn't the IDL. The win is NOT using reflection to send messages . You can manually implement the message reading/writing without reflection just as well but will take a lot more work.

So it's more than just maintaining struct imho.

@nemith
Copy link
Author

nemith commented Apr 13, 2024

Remember that jute define not only the structs but also the encoding.

@nemith
Copy link
Author

nemith commented Apr 13, 2024

More evidence:

with existing reflection encoding

package zk

import "testing"

func BenchmarkPacketEncode(b *testing.B) {
	var buf []byte

	packet := connectRequest{
		ProtocolVersion: 0,
		LastZxidSeen:    0,
		TimeOut:         2000,
		SessionID:       0,
		Passwd:          []byte("foo"),
	}

	for i := 0; i < b.N; i++ {
		encodePacket(buf, &packet)
	}
}

with jute (no reflection) encoding

$ cat bench_test.go
package proto

import (
        "bytes"
        "testing"

        "github.com/go-zookeeper/jute/lib/go/jute"
)

func BenchmarkEncode(b *testing.B) {
        var buf bytes.Buffer
        enc := jute.NewBinaryEncoder(&buf)

        packet := ConnectRequest{
                ProtocolVersion: 0,
                LastZxidSeen:    0,
                TimeOut:         2000,
                SessionId:       0,
                Passwd:          []byte("foo"),
        }

        for i := 0; i < b.N; i++ {
                enc.Encode(&packet)
        }
}

Results

$ go test . -bench=BenchmarkPacketEncode -run=NONE
goos: darwin
goarch: arm64
pkg: github.com/go-zookeeper/zk
BenchmarkPacketEncode-10         4097984               286.3 ns/op
PASS
ok      github.com/go-zookeeper/zk      1.629s

$ go test . -bench=BenchmarkEncode -run=NONE
goos: darwin
goarch: arm64
pkg: github.com/go-zookeeper/zk/internal/proto
BenchmarkEncode-10      30879982                38.96 ns/op
PASS
ok      github.com/go-zookeeper/zk/internal/proto       1.631s

A 7.3x improvement may be worth it. Especially that all the code gen stuff is already done. It's literally just repumbing with new messages. The reason it hasn't been done yet is that messages are exported and it's a breaking change.

This also gets better as the old version writes (and allocates) buffers on each write where the new one can write directly to a buffered socket.

The IDL sucks, but so does reflection.

@nemith
Copy link
Author

nemith commented Apr 13, 2024

For compat you can look into making the generated code more compatible or just writing translations between existing structs (my plan before i left FB) . Ideally structs are not exported and them being exported is also a mistake. Both can be fixed in a proper v2 version (which there is a lot of api that needs cleaning up)

@jeffbean
Copy link

good points. I'm thinking how to sustain this lib and adding this adds a bit of complexity. That being said since the jute stuff most likely is also frozen, once the work to do this is done its most likely not going to change from underneath us.

v2 I'm not sure what it really looks like yet ... I'm kind of hoping to get a bunch of the v1 bugs and PRs cleaned up and cut a v2 dev or something where we can start to break things in serious ways.

I'm just warming back up to be active once again, jute was the thing I am most unfamiliar and uncomfortable supporting. If the code gen is in a good place then yeah, its a big win

@pmazzini pmazzini removed their assignment May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants