Not able to marshal a slice of large structs

[abiliojr]

I'm trying to marshal an "heterogeneous" slice as an experiment (one of interfaces), and I'm hitting a wall with it. If I fill it with structs of up to 3 elements, it will encode them fine. If I add a single struct with 4 elements, it fails at runtime, saying:

msgp: type "main.FailureObject" not supported

FailureObject being the problematic struct in my example to reproduce the problem, though I proved I can marshal an individual object of that type.

Note: I intentionally need the objects to be encoded as arrays, and that's why I'm adding the "tuple" on top.

I'm completely new in Go, so most probably is my mistake. Anyhow, can you please take a look at the code:

package main

import "fmt"

//go:generate msgp
type ListOfObjects []interface{}

//msgp:tuple OkObject
type OkObject struct {
	A uint8
	B uint8
	C uint8
}

//msgp:tuple FailureObject
type FailureObject struct {
	A uint8
	B uint8
	C uint8
	D uint8
}

func AppendOkObj(list ListOfObjects, b uint8, c uint8) ListOfObjects {
	return append(list, OkObject{0, b, c})
}

func AppendFailureObj(list ListOfObjects, b uint8, c uint8) ListOfObjects {
	return append(list, FailureObject{1, b, c, 1})
}

func main() {
	// This will work. I can marshal a "long" object
	thisWorks := FailureObject{1, 2, 3, 4}
	res, _ := thisWorks.MarshalMsg(nil)
	fmt.Printf("% X\n", res)

	// I can do this and it will work
	list := make(ListOfObjects, 0, 10)
	list = AppendOkObj(list, 0, 0)
	list = AppendOkObj(list, 1, 1)
	list = AppendOkObj(list, 2, 2)
	res, _ = list.MarshalMsg(nil)
	fmt.Println("as you can see, it works")
	fmt.Printf("% X\n", res)

	// now this will give an error and instead return a half baked message
	list = make(ListOfObjects, 0, 10)
	list = AppendFailureObj(list, 0, 0)
	list = AppendFailureObj(list, 0, 0)
	res, err := list.MarshalMsg(nil)
	fmt.Println("as you can see, this doesn't")
	fmt.Printf("% X\n", res)
	fmt.Println(err)

	// combination of short and long won't work either
	list = make(ListOfObjects, 0, 10)
	list = AppendOkObj(list, 1, 1)
	list = AppendFailureObj(list, 0, 0)
	res, err = list.MarshalMsg(nil)
	fmt.Println("neither this")
	fmt.Printf("% X\n", res)
	fmt.Println(err)
}

If I'm assuming something here that is wrong, what would be the correct way, cleanest and most efficient way of encoding an array of arrays that can have different sizes and that are mapped to existing types.

[erikdubbelboer]

The problem is that it generates a method receiver on the pointer type:

func (z *FailureObject) EncodeMsg(en *msgp.Writer) (err error) {

While for OkObject it's on the value type:

func (z OkObject) EncodeMsg(en *msgp.Writer) (err error) {

The reason why is this piece of code:

msgp/gen/spec.go

Lines 233 to 244 in 53e4ad1

	case *Struct:
	// TODO(HACK): actually do real math here.
	if len(e.Fields) <= 3 {
	for i := range e.Fields {
	if be, ok := e.Fields[i].FieldElem.(*BaseElem); !ok || (be.Value == IDENT || be.Value == Bytes) {
	goto nope
	}
	}
	return p.TypeName()
	}
	nope:
	return "*" + p.TypeName()

Which was introduced in commit cd70faa which says allow some encode/marshal method recvrs to be values.

The 3 here makes the difference why your OkObject works and FailureObject fails.

I don't understand where the 3 comes from or what for loop is actually checking but changing it to just:

case *Struct:
  return p.TypeName()

still passes all tests and fixes this bug.

In theory all receivers for EncodeMsg should be able to be values seeing as it doesn't change the object.

@philhofer do you remember writing this code 3 years ago and why you did it this way?

[philhofer]

So, originally I wrote the code to always use pointer receivers, since there's sometimes a substantial performance penalty to copying the whole struct into the method. Then someone complained that they wanted to use value receivers. The middle-ground that the code sits in right now is that it uses
value receivers for "small" structs (three words or under, so smaller than a slice header), and uses pointer receivers otherwise.

Looking back on things, I should have just stuck with pointer receivers, because the behavior is not intuitive or well-documented.

[abiliojr]

I increased the value from 3 to 10 and now it works. Anything I can do to help?

[philhofer]

You likely don't want to be copying (or boxing) 80-byte structures.

The simplest fix for you is:

func AppendFailureObj(list ListOfObjects, b uint8, c uint8) ListOfObjects {
-	return append(list, FailureObject{1, b, c, 1})
+	return append(list, &FailureObject{1, b, c, 1})
}

You can cause your example to fail during type-checking (rather than at run-time) by making your slice of type []msgp.Marshaler rather than []interface{}.

[abiliojr]

I understand the first suggestion, but the second one isn't obvious to me (I read the wiki a couple of weekends ago actually looking for some detail on how to marshal a slice, and I don't remember reading anything like that, then I learned about slices of interfaces as a concept).

Anyhow, I tried []msgp.Marshaler, and now I get a different error:
demo.go: ListOfObjects: unresolved identifier: msgp.Marshaler

I'm using it as:
type ListOfObjects []msgp.Marshaler

What am I doing wrong?

As a suggestion, I think this info can be in the tips and tricks section of the wiki (unless you consider it too obvious). I would write it myself in order to help, but with less than 10 hours of experience, I don't consider it wise ;)

[philhofer]

You'll need import "github.com/tinylib/msgp/msgp" in order to use the msgp.Marshaler identifier.

[abiliojr]

I was really impressed about the editor I'm using (vscode) because it automatically detected the need for the import and added it. But still, I get that error message when I try to manually run:
~/go/bin/msgp --file demo.go

[philhofer]

The full import is "github.com/tinylib/msgp/msgp" (note the second 'msgp'; the top-level one is the code generation tool, not a library). Your editor is probably selecting the wrong import.

[philhofer]

Also, keep in mind that "unresolved identifier" is just a warning produced by the tool when it encounters a type not defined in the input file, not an error. But you'll probably want to avoid generating methods for that type.

[abiliojr]

Yes, the editor actually included the right one (two msgp). Here a simplified example:

package main

import (
	"github.com/tinylib/msgp/msgp"
)

//go:generate msgp
type ListOfObjects []msgp.Marshaler

//msgp:tuple Obj
type Obj struct {
	A uint8
	B uint8
	C uint8
}

func AppendObj(list ListOfObjects, b uint8, c uint8) ListOfObjects {
	return append(list, Obj{0, b, c})
}

func main() {
	list := make(ListOfObjects, 0, 10)
	list = AppendObj(list, 0, 0)
	res, _ := list.MarshalMsg(nil)
}

The warning prints as usual, but if I run go build, I get compiler errors:

./demo_gen.go:22:21: (*z)[zb0001].DecodeMsg undefined (type msgp.Marshaler has no field or method DecodeMsg)
./demo_gen.go:37:18: z[zb0003].EncodeMsg undefined (type msgp.Marshaler has no field or method EncodeMsg)
./demo_gen.go:71:26: (*z)[zb0001].UnmarshalMsg undefined (type msgp.Marshaler has no field or method UnmarshalMsg)
./demo_gen.go:84:17: z[zb0003].Msgsize undefined (type msgp.Marshaler has no field or method Msgsize)

[philhofer]

Stick a //msgp:ignore ListOfObjects and you should be fine.

(It's not possible to generate code for the ListOfObjects type.)

[erikdubbelboer]

@philhofer is this something you are still willing to change?

You likely don't want to be copying (or boxing) 80-byte structures.

That actually takes only a couple of nanoseconds. I wrote a simple test: https://gist.github.com/erikdubbelboer/98c8a910b1151c6df0e1ecba56fffcec
The result:

BenchmarkValue-8     	50000000	        28.0 ns/op	       0 B/op	       0 allocs/op
BenchmarkPointer-8   	100000000	        22.9 ns/op	       0 B/op	       0 allocs/op

As you can see the difference is neglectable.

[abiliojr]

After adding the ignore as per instructions, the warning disappears as expected, but when I try to compile I still get an error:
./demo.go:27:16: list.MarshalMsg undefined (type ListOfObjects has no field or method MarshalMsg)

Which I kind of understand as valid

[erikdubbelboer]

@philhofer what you want isn't possible. Since it's a slice you have to generate code for it. But the generated code assumes each element has the methods DecodeMsg, EncodeMsg, UnmarshalMsg and Msgsize which msg.Marshaler obviously doesn't have.

The closest you can get is:

type ListType interface {
  msgp.Decodable
  msgp.Encodable
  msgp.Marshaler
  msgp.Unmarshaler
  msgp.Sizer
}

type ListOfObjects []ListType

But this results in a:

./test.go:24:15: cannot use Obj literal (type Obj) as type ListType in append:
	Obj does not implement ListType (DecodeMsg method has pointer receiver)

[erikdubbelboer]

I suggest changing msgp to always use a value receiver just like json.RawMessage for example.