packet: use [188]byte as Packet type #84

kortschak · 2018-05-30T00:47:18Z

NOT FOR SUBMISSION WITHOUT DISCUSSION

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 · 2018-05-30T00:47:24Z

All committers have signed the CLA.

kortschak · 2018-05-30T00:48:24Z

packet/adaptationfield/create.go

@@ -4,13 +4,13 @@ import "github.com/Comcast/gots/packet"

 func SetPrivateData(pkt *packet.Packet, af []byte) {


Note that the potential for failure here suggests that SetPrivateData should return an error.

guygrigsby · 2018-05-31T19:41:12Z

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 · 2018-05-31T21:59:40Z

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 · 2018-05-31T22:13:43Z

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).

guygrigsby

Overall, I think that there is a lot of value in the compile time checks and the elimination of the length check for every Packet operation. The real problem is that it breaks all dependent projects. I would like to get opinions from those projects. @paulboschert @mikereedell @jessejlt @ianeckart @davemt @tmm1 @stilldavid Can any of you offer an opinion?

guygrigsby · 2018-06-13T19:14:35Z

packet/accumulator.go

-	emptyByteArray []byte
-)
-
-type doneFunc func([]byte) (bool, error)


Why eliminate this type?

It gives you very little. The only thing it saves you from is another named function type, but this is unexported to the surface is minuscule and it causes readability and documentation to suffer.

To expand on this, your NewAccumulator takes a doneFunc, but what is this? A users needs to read the source because it's unexported and so doesn't show up in the godoc. And because it's unexported they have to use a func([]byte) (bool, error) anyway which is assignable to a doneFunc but is not the same type. If it were exported there would be a marginal benefit, but still it would impact on readability since "What is a packet.DoneFunc anyway?.... [navigates to definition... Ah, OK." and still most people will use an unnamed type closure or function value anyway.

I think there is some value in the label doneFunc as it makes it easier to understand what is happening. That label can be used in accumulator and NewAccumulator instead.

Mmmm. What I'd do would be NewAccumulator(func(data []byte) (done bool, err error)) Accumumulator, which explains far more than what currently exists.

BTW @guygrigsby Why is accumulator hidden behind an Accumulator? Is there a plan for alternative implementations?

I have no plan for any additional implementations. IIRC, the possibility existed when we created it, but obviously that has not come to be. It's possible that doneFunc makes sense to me because I am used to starting at it. I am a big fan of self documenting code and I think that func types are a great way to do that.

WRT doneFunc, it actually reduced code documentation except for people reading the source, and moves the identity of the type away from the use, to the definition of the type. To demonstrate, read the godoc for packet.NewAccumulator. Now, as a person who wants to use packet.NewAccumulator, but has not read the source and is new to gots (there are many Go users who don't read source of packages they use - I don't understand why, but it's a thing), exactly what is a doneFunc type? The documentation is completely absent.because the type not exported, so you would need to navigate to the package.NewAccumulator definition in the source in the hope that type doneFunc is defined near near it, which in this case it is-ish. If you look at the case in the stdlib that take func types, generally the types are only named types when they have methods on them, or they are usually not user-facing types http.HandlerFunc is an obvious case of user-facing named func type, but it has a method.

For the use of interfaces here, I wouldn't. Again this use reduced the visibility of documentation because you lose links from the godoc to method implementations. You can see this by navigating to packet.Accumulator. If it were a concrete type, you would be able to follow the links to source. This doesn't help those mentioned above who don't read the source, but is a boon for people who do when the documentation is not helpful enough.

BTW There is no handling of the case of a nil parameter here. Is there a sensible default? If not, it should probably be guarded and NewAccumulator should return an error.

Also, I'm wondering if it's not more sensible to just have the packets and ffields exported. Is there anything you are particularly protecting against there?

guygrigsby · 2018-06-13T19:18:45Z

packet/accumulator_test.go

@@ -38,7 +38,7 @@ func ExamplePacketAccumulator() {

 	secondPacket, _ := hex.DecodeString("47006411f0002b59bc22ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff")

-	packets := []Packet{firstPacket, secondPacket}
+	packets := [][]byte{firstPacket, secondPacket}


There are a few changes like this. Changing to []byte from Packet. What's the benefit? We lose the compile time checking, which is why the change to [188]byte would be so beneficial, IMO.

Here it's not a packet, it's a []byte, having been returned by a stdlib function that returns []byte. This change is in tests.

guygrigsby · 2018-06-13T19:26:49Z

packet/create.go

 }

 // WithHasPayloadFlag is an option function for creating a packet with a payload flag
 func WithHasPayloadFlag(pkt *Packet) {
-	(*pkt)[3] = byte((*pkt)[3] | 0x10)
+	pkt[3] |= 0x10


I didn't even know this was a thing. :)

Array pointers are indexable - this is very useful for range where otherwise you'd copy.

guygrigsby · 2018-06-13T19:28:03Z

packet/create.go

+	b[3] = byte(pts >> 7 & 0x7f)
+	b[2] = byte(pts >> 14 & 0xff)
+	b[1] = byte(pts >> 22 & 0x4f)
+	b[0] = byte(pts >> 29 & 0x07)


What do we gain by this change?

Elision of subsequent bounds checks.

guygrigsby · 2018-06-13T19:29:52Z

packet/packet_test.go

+	}
+	pkt := new(Packet)
+	if copy(pkt[:], b) != PacketSize {
+		panic("bad test (wrong length): " + h)


I know this is only a test, but I would prefer not to panic

There are two levels of failure in a test, failure of the code to perform the tests correctly and failure of the tests to do what they are believed to do. This checks for the latter and should shout at you. The input to this is static compile time (authoring time actually) entity., which is exactly what panics were designed for.

tmm1 · 2018-06-13T19:39:53Z

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 · 2018-06-29T11:06:24Z

ping

guygrigsby · 2018-07-09T22:54:47Z

@jessejlt Thoughts?

WillGunther · 2018-07-19T19:09:04Z

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 · 2018-07-19T20:52:52Z

Thanks. Done.

Let me know when you are ready.

kortschak · 2018-08-01T11:50:57Z

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.

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.

WillGunther · 2018-08-01T15:35:02Z

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.

mikereedell · 2018-08-03T16:02:59Z

psi/pmt_test.go

@@ -33,6 +33,18 @@ import (
 	"github.com/Comcast/gots/packet"
 )

+func parseHexString(h string) *packet.Packet {


I don't like how this function is declared in three separate places, but that's something that can be addressed later.

WillGunther · 2018-08-03T16:07:10Z

Alright! Finally merging. Thanks for your patience @kortschak

kortschak · 2018-08-03T21:21:48Z

Thanks for the review.

kortschak commented May 30, 2018

View reviewed changes

guygrigsby reviewed Jun 13, 2018

View reviewed changes

kortschak force-pushed the arraypackets branch from 42fffbc to bde87bb Compare June 29, 2018 11:10

kortschak force-pushed the arraypackets branch from bde87bb to 3443605 Compare July 19, 2018 20:52

kortschak added 2 commits August 1, 2018 21:25

packet: improve NewAccumulator documentation

94115b6

kortschak force-pushed the arraypackets branch from 3443605 to 94115b6 Compare August 1, 2018 11:55

mikereedell approved these changes Aug 3, 2018

View reviewed changes

WillGunther merged commit b075ace into Comcast:master Aug 3, 2018

kortschak deleted the arraypackets branch August 3, 2018 21:21

This was referenced Aug 3, 2018

packet: propose Packet be changed to [188]byte #83

Closed

packet/adaptationfield: private data length is not safely handled #82

Closed

mikereedell mentioned this pull request Jan 16, 2019

Removing unncecessary multi-value returns when the returned err is always nil. #117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

packet: use [188]byte as Packet type #84

packet: use [188]byte as Packet type #84

kortschak commented May 30, 2018

CLAassistant commented May 30, 2018 •

edited

Loading

kortschak May 30, 2018

guygrigsby commented May 31, 2018

kortschak commented May 31, 2018

kortschak commented May 31, 2018 •

edited

Loading

guygrigsby left a comment

guygrigsby Jun 13, 2018

kortschak Jun 13, 2018

tmm1 Jun 13, 2018

kortschak Jun 13, 2018

guygrigsby Jun 14, 2018

kortschak Jun 14, 2018 •

edited

Loading

kortschak Jun 14, 2018

guygrigsby Jun 13, 2018

kortschak Jun 13, 2018

guygrigsby Jun 13, 2018

kortschak Jun 13, 2018

guygrigsby Jun 13, 2018

kortschak Jun 13, 2018

guygrigsby Jun 13, 2018

kortschak Jun 13, 2018

tmm1 commented Jun 13, 2018

kortschak commented Jun 29, 2018

guygrigsby commented Jul 9, 2018

WillGunther commented Jul 19, 2018

kortschak commented Jul 19, 2018

kortschak commented Aug 1, 2018 •

edited

Loading

WillGunther commented Aug 1, 2018

mikereedell Aug 3, 2018

WillGunther commented Aug 3, 2018

kortschak commented Aug 3, 2018

		@@ -4,13 +4,13 @@ import "github.com/Comcast/gots/packet"

		func SetPrivateData(pkt *packet.Packet, af []byte) {

packet: use [188]byte as Packet type #84

packet: use [188]byte as Packet type #84

Conversation

kortschak commented May 30, 2018

CLAassistant commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

guygrigsby commented May 31, 2018

kortschak commented May 31, 2018

kortschak commented May 31, 2018 • edited Loading

guygrigsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortschak Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmm1 commented Jun 13, 2018

kortschak commented Jun 29, 2018

guygrigsby commented Jul 9, 2018

WillGunther commented Jul 19, 2018

kortschak commented Jul 19, 2018

kortschak commented Aug 1, 2018 • edited Loading

WillGunther commented Aug 1, 2018

Choose a reason for hiding this comment

WillGunther commented Aug 3, 2018

kortschak commented Aug 3, 2018

CLAassistant commented May 30, 2018 •

edited

Loading

kortschak commented May 31, 2018 •

edited

Loading

kortschak Jun 14, 2018 •

edited

Loading

kortschak commented Aug 1, 2018 •

edited

Loading