Use pointer receiver when Marshalling and Encoding
msackman opened this issue · 6 comments
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.
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).
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.
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.
Thank you. I'm afraid I decided to go a different direction - https://fossil.wellquite.org/bebop
Alright, well I'll have to make mine faster :)