packet: use [188]byte as Packet type

[kortschak]

NOT FOR SUBMISSION WITHOUT DISCUSSION

See https://github.com/Comcast/gots/issues/83#issuecomment-392954730

This obviates a collection of runtime testing (moving it to compile time), removes a few test cases that can no longer fail, likely improves performance by eliding bounds checks in constant index look-ups, reduces space very slightly and allows additional uses of Packet that were not previously available (though possibly not useful: e.g. using Packet as a map key).

A number of other unrelated changes were made that simplify code and reduce line count.

[CLAassistant]

All committers have signed the CLA.

[guygrigsby]

A quick review of this really brings to light some good stuff. I am going to need some time to take a proper look and do some testing.

[kortschak]

Yeah, no worries. I'm happy to discuss my thinking in this. Even if there is not a desire to move to the statically sized type, there are a lot of changes here in style that you should probably make.

[kortschak]

While I'm thinking about this, there's a slight increase in buffer copying here which may or may not have a performance impact that you care about, so benchmarking is worthwhile (there are likely wins that may make up for it). There is a possibility of doing zero-copy conversion using unsafe, but that brings with it a risk of some memory over use if clients are using the cap-slicing operator. I think that in general, the copying is probably the better approach even ignoring that issue, but for posterity:

func unsafePacket(b []byte) *Packet {
    if len(b) != len(Packet{}) || cap(b) != len(Packet{}) {
        panic("packet: invalid unsafe byte slice to packet conversion")
    }
    return *(**Packet)(unsafe.Pointer(&b))
}

This probably should not be used, and if it is, only for converting []byte -> *Packet where you have complete control of the []byte (this is true in a few places).

[tmm1]

I think this is worth the API breakage, but I am a relatively new user and don't have a lot of code using the library.

[kortschak]

ping

[guygrigsby]

@jessejlt Thoughts?

[WillGunther]

So I think there is some consensus that these are good changes. We are waiting to get a review from one of the main internal consumers of this library, and then if they sign off we will merge it. @kortschak if you get a chance could you update the branch?

[kortschak]

Thanks. Done.

Let me know when you are ready.

[kortschak]

Is there a timeline for a response here? I have fixed the conflicts, but now I need to do that again and given the existence of #92, I'm concerned that I will have to do that again, with a significant amount of work the result.

[WillGunther]

Right, sorry about the delay. We are still waiting on that one review. Holding off on all merges until that review comes through. We have gotten a commitment that this will be reviewed by the end of the week. I've also talked to the author of #92 and he is willing to make the changes on his side for the 188 bytes.

[WillGunther]

Alright! Finally merging. Thanks for your patience @kortschak

[kortschak]

Thanks for the review.