Opportunistically decode attributes in AttributeDecoder

[ti-mo]

Hi Matt,

As discussed yesterday, this patch contributes two things:

• adds AttributeDecoder.TypeFlags() that returns the Type with an inverted attrTypeMask
• inverts the call chain between UnmarshalAttributes() and ad.Next()

This change is motivated by ti-mo/conntrack#13, as I'd like to expose a netfilter.AttributeDecoder/Encoder and conntrack.AttributeDecoder/Encoder interface all the way up the call chain. If done right, decoding a whole netlink message can be done with only a single allocation of each layer's Attribute type. 😱

It doesn't save much on the benches in the repo, because it decodes all attributes anyway, but it allows for large efficiency gains in downstream libraries. Because attributes can now be decoded opportunistically, (eg. calling ad.Next() until you have the attributes you care about and calling it quits), a caller that uses AttributeDecoder will no longer implicitly decode the whole message. This potentially saves a lot of CPU time.

I also cut down on memory allocations, since AttributeDecoder now uses a single netlink.Attribute as a scratch buffer it repeatedly unmarshals into, completely removing allocations from the hot path.

This is illustrated by the benchmark results.

Before:

λ  netlink 38c649d ✓ go test -bench BenchmarkUnmarshalAttributes
goos: linux
goarch: amd64
pkg: github.com/mdlayher/netlink
BenchmarkUnmarshalAttributes/0-16  	354637596	         3.13 ns/op	       0 B/op	       0 allocs/op
BenchmarkUnmarshalAttributes/1-16  	 8755629	       134 ns/op	      33 B/op	       2 allocs/op
BenchmarkUnmarshalAttributes/8-16  	 1403520	       856 ns/op	     544 B/op	      12 allocs/op
BenchmarkUnmarshalAttributes/64-16 	  182522	      6417 ns/op	    8160 B/op	      71 allocs/op
BenchmarkUnmarshalAttributes/512-16         	   10000	    114465 ns/op	  294880 B/op	     522 allocs/op

After:

λ  netlink master ✓ go test -bench BenchmarkUnmarshalAttributes
goos: linux
goarch: amd64
pkg: github.com/mdlayher/netlink
BenchmarkUnmarshalAttributes/0-16  	13132034	        92.4 ns/op	     112 B/op	       1 allocs/op
BenchmarkUnmarshalAttributes/1-16  	 5177278	       234 ns/op	     145 B/op	       3 allocs/op
BenchmarkUnmarshalAttributes/8-16  	 1474484	       806 ns/op	     432 B/op	      10 allocs/op
BenchmarkUnmarshalAttributes/64-16 	  183102	      6580 ns/op	    6256 B/op	      66 allocs/op
BenchmarkUnmarshalAttributes/512-16         	   10000	    103788 ns/op	  278640 B/op	     514 allocs/op

The extra allocation visible in 0 and 1 is due to an added make([]Attribute, 0, ad.Len()) in UnmarshalAttributes that pre-allocates the backing array of the output slice. This amortizes nicely over time.

The existing test suite was very helpful in weeding out any inconsistencies in behaviour, so for example I had to add an extra check in UnmarshalAttributes to make sure it explicitly returns a nil []Attribute when no attributes where decoded.

Let me know what you think!

[ghost]

Found some fixes!

P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).

[mdlayher]

LGTM overall, just a couple of nits. I assume your fuzzing is going well?

[ti-mo]

LGTM overall, just a couple of nits. I assume your fuzzing is going well?

At about an hour now, had to interrupt but resumed from the same corpus.

2019/12/06 16:00:16 workers: 16, corpus: 76 (2m46s ago), crashers: 0, restarts: 1/9994, execs: 301119156 (149143/sec), cover: 87, uptime: 33m39s

Thanks for the review!

[ti-mo]

Fuzzer ran for 4.5hrs without any crashers. ☝️

[ti-mo]

Looks like this implements #115!

[florianl]

looks all good :)

[mdlayher]

LGTM!