200sc / bebop

bebop wire format in Go
Apache License 2.0
71 stars 5 forks source link

Use pointer receiver when Marshalling and Encoding #33

Closed msackman closed 12 months ago

msackman commented 1 year ago

Currently, the generator creates:

func (bbp Foo) MarshalBebopTo(buf []byte) int {...}
func (bbp Foo) EncodeBebop(iow io.Writer) (err error) {...}

By having a non-pointer receiver, it creates a copy of the struct, using memory, which I think is unnecessary. Is there any reason why this was chosen? Looking at some generated functions, it doesn't look like it modifies anything in those structs... If these were changed to:

func (bbp *Foo) MarshalBebopTo(buf []byte) int {...}
func (bbp *Foo) EncodeBebop(iow io.Writer) (err error) {...}

then it would avoid unnecessary allocs.

msackman commented 1 year ago

I've made the changes locally, and all your tests still pass... so I can send a PR if you think this idea is safe. I've changed the Getters and Size methods too, for the same reason (and consistency).

200sc commented 1 year ago

The justification was: "These methods do not modify the object, and so they should not use a pointer". A pointer is (usually) 64 bits, we could assume that structs over this size perform worse and below this size perform better when they do not use a pointer for their methods; the majority of structs are likely over this size so we could assume this is a general performance improvement.

@msackman If you have a changeset ready please open a PR and I'll review it, it sounds like a valid improvement. If we wanted to be extremely thorough we could benchmark it or offer it as a configuration option, but I'm tempted to say those can be future steps if anyone demonstrates a desire for the current behavior.

msackman commented 1 year ago

Cool, created PR #34

200sc commented 12 months ago

Just to note, the PR and change is still fine if it is done and passes tests; I'm still monitoring this repository and plan to add features as the upstream supports them.

msackman commented 12 months ago

Thank you. I'm afraid I decided to go a different direction - https://fossil.wellquite.org/bebop

200sc commented 12 months ago

Alright, well I'll have to make mine faster :)