Added function for lazy message reading per #222 #224

asergeyev · 2015-06-24T19:12:47Z

No description provided.

miekg · 2015-06-24T19:39:34Z

As I think is my normal review mode, i will go over each change in detail and then (hopefully) see the bigger picture and moan about that :-)

miekg · 2015-06-24T19:39:56Z

client.go

 		return nil, err
+	} else if n < 12 {


Think 12 is better as a constant, headerSize ?

Any reason to choose private vs making it public?

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

return nil, err

} else if n < 12 {

Any reason to choose private vs making it public?

Could as well be public.

miekg · 2015-06-24T19:47:31Z

Looks pretty good. Actually hooking it up in the lib itself is another PR (which is really fine)

asergeyev · 2015-06-25T03:34:50Z

Should I continue in this one to reuse Msg.Unpack properly or finish this one and open new later?

miekg · 2015-06-26T06:17:34Z

Fixing this up first and then a new PR is fine. Does this function need to be exported though?

asergeyev · 2015-06-26T13:11:01Z

Does this function need to be exported though?

Yeah... it's good function for convenience of those who write clients. Sometimes you only need packets with answers and without this function you'd go and parse all fields in authority/additional even if your client does not care about them.

miekg · 2015-06-26T13:52:53Z

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

Does this function need to be exported though?

Yeah... it's good function for convenience of those who write clients. Sometimes you only need packets with answers and without this function you'd go and parse all fields in authority/additional even if your client does not care about them.

Ok. Works for me.

/Miek

Miek Gieben

asergeyev · 2015-06-29T00:27:18Z

Well... Here is what I got...

I still followed with changing all we discussed in one PR, since there is not much. Logic is still same as it was in ReadMsg.

I'm not sure there is any good in pursuing creation of a function that's not reserving such large buffers... In TCP it's probably doable, but in UDP things must be loaded in one Read call; it's really not possible to change...

miekg · 2015-06-29T06:44:16Z

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

Well... Here is what I got...

I still followed with changing in one PR, since there is not much changed as of now. Logic is still same. I'm not sure there is any good in pursuing creation of a function that's not reserving such large buffers... In TCP it's probably doable, but in UDP things should be loaded in one read call so it's really not possible to change...

Ah yeah. I see what you mean... you can't do a partial read and then
read-some-more when you're ready for it... Part of the appeal of such
a function it that it would be nice if you could. Not sure how though...

asergeyev · 2015-06-29T12:09:37Z

Last patch shows how I see it could be done for TCP. 64K is very large buffer I agree. dns.Conn.Read becomes less useful here since it expects to read everything in one pre-allocated buffer in TCP.

miekg · 2015-06-30T20:08:59Z

client.go

+//
+// Note that this function would not be able to report TSIG error or
+// check it got actual DNS payload.
+func (co *Conn) ReadMsgBytes(hdr *Header) ([]byte, error) {


So... What if we would really makes this only read 12 (or 14 in case of tcp) bytes and let it return the Conn, the bytes read and an error? Maybe the buffer should also by given to the function, so it is really without any internal allocations? So if you get an error you just discard and write FormErr (maybe from within the lib already). If thinks look OK we can parse the bytes to a real header and assemble the packet...

Once you read from UDP rest is discarded. You could not really read again there so you end up buffering and faking ability to read which is slow.

http://golang.org/src/net/udpsock_posix.go?s=1547:1620#L53
http://golang.org/src/net/fd_unix.go#L256

then from rcvfrom(2):

All three routines return the length of the message on successful completion. If a message is too long to fit in the supplied buffer, excess bytes may be discarded depending on the type of socket the message is received from.

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

+// Note that this function would not be able to report TSIG error or
+// check it got actual DNS payload.
+func (co *Conn) ReadMsgBytes(hdr *Header) ([]byte, error) {

Once you read from UDP rest is discarded. You could not really read again there so you end up buffering and faking ability to read which is slow.

http://golang.org/src/net/udpsock_posix.go?s=1547:1620#L53
http://golang.org/src/net/fd_unix.go#L256

then from rcvfrom(2):

All three routines return the length of the message on successful completion. If a message
is too long to fit in the supplied buffer, excess bytes may be discarded depending on the
type of socket the message is received from.

Grrrr :( So what would be the win then, this is the typical space/time trade
off? So the allocation of memory still happens, the only thing that you shortcut
is the potentially slowish parsing of the rest of the packet. I rather spend
cycles on making that faster, or fail ealier.

/Miek

Miek Gieben

ReadMsgBytes -> ReadMsgHeader as a function name.

asergeyev · 2015-07-01T12:34:27Z

I'll experiment with profiler a bit and let you know. IMO net lib has limits by itself plus fact that dns lib built around strings... Allocs for message reading are probably not worst thing there....

asergeyev · 2015-07-01T13:38:18Z

I created new clientbench branch and put some test code there... As expected, UDP is close, almost no difference but TCP is faster in new implementation (not reserving 64K each time):

BenchmarkNewUDPClient      10000        342285 ns/op
BenchmarkOldUDPClient       5000        347112 ns/op
BenchmarkNewTCPClient      10000        132508 ns/op
BenchmarkOldTCPClient      10000        202532 ns/op

With pprof and web -cum BenchmarkName it looks that most CPU time was still on syscalls... and pack/unpack StructValue; I decided to stub all of those, wrote small fake net.Conn for that... (second commit in that branch)... I also changed UDPSize to 4096 and run it with benchtime=10s to make sure I'll have small CPU expenses visible (paranoid Alex).

Here is what it looks like for "web -cum ReadMsg" only on new client test:

 https://www.dropbox.com/s/3iw1o23ytnksvie/pprof0002.svg?dl=1

You can see there is almost no time spent where you think we can win something.

miekg · 2015-07-02T08:45:11Z

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

I created new clientbench branch and put some test code there... As expected, UDP is close, almost no difference but TCP is faster in new implementation (not reserving 64K each time):

BenchmarkNewUDPClient 10000 342285 ns/op
BenchmarkOldUDPClient 5000 347112 ns/op
BenchmarkNewTCPClient 10000 132508 ns/op
BenchmarkOldTCPClient 10000 202532 ns/op

With pprof and web -cum BenchmarkName it looks that most CPU time was still on syscalls... and pack/unpack StructValue; I decided to stub all of those, wrote small fake net.Conn for that... (second commit in that branch)... I also changed UDPSize to 4096 and run it with benchtime=10s to make sure I'll have small CPU expenses visible (paranoid Alex).

Here is what it looks like for "web -cum ReadMsg" only on new client test:
https://www.dropbox.com/s/3iw1o23ytnksvie/pprof0002.svg?dl=1
You can see there is almost no time spent where you think we can win something.

Ack. Yep, that points to something different.

asergeyev · 2015-07-02T11:04:17Z

But convinience that I was asking for in #222 is already there plus small bonus for TCP... Maybe you can merge this one? (I understand if not...)

as of performance, could be separate effort to look at different places but the need of reserving smallish structures with new(...) (or &Name{}) and work with strings is what could be hard to deal with...
And, since your lib is not just wire client but also works with text representation a lot I do not think you should change the approach.

miekg · 2015-07-02T11:49:09Z

[ Quoting notifications@github.com in "Re: [dns] Added function for lazy m..." ]

But convinience that I was asking for in #222 is already there plus small bonus for TCP... Maybe you can merge this one? (I understand if not...)

Yes, I will. But I first want to take another look.

as of performance, could be separate effort to look at different places but the
need of reserving smallish structures with new(...) (or &Name{}) and work with
strings is what could be hard to deal with... And, since your lib is not just
wire client but also works with text representation a lot I do not think you
should change the approach.

No, indeed. Having just strings make it easy to work with.

/Miek

Miek Gieben

miekg · 2015-07-02T21:58:34Z

client.go

+	return m, err
+}
+
+// ReadMsgBytes reads DNS packet, parses and fills wire header (passing nil


ReadMsgBytes reads a DNS message, parses it and populates hdr (when hdr is not nil)

miekg · 2015-07-02T21:59:12Z

client.go

+}
+
+// ReadMsgBytes reads DNS packet, parses and fills wire header (passing nil
+// would cause skipping that). Returns message in byte format to parse with


It return the rest of the message as a byte slice to pasrse with Msg.Unpack later on.

asergeyev · 2015-07-03T02:40:28Z

Thanks for careful review... I decided not to wait on your comments, I think I followed most of your recommendations here.

miekg · 2015-07-03T08:24:20Z

Ack, two naming nits. But it fix those shortly.

Added function for lazy message reading per #222

asergeyev mentioned this pull request Jun 24, 2015

Lazy message reading in dns.Client #222

Closed

miekg reviewed Jun 24, 2015
View reviewed changes

asergeyev added 2 commits June 28, 2015 20:21

Added function for lazy message reading per miekg#222

2f3bcbd

Shortened ReadMsg using ReadMsgBytes, added a constant.

53dfadf

asergeyev force-pushed the master branch from 1f77ed6 to 53dfadf Compare June 29, 2015 00:22

Not allocating 64K buffers for reading

b625f19

miekg reviewed Jun 30, 2015
View reviewed changes

miekg reviewed Jul 2, 2015
View reviewed changes

Fixed things found in code review (comments, semantics).

b0d6ff3

miekg added a commit that referenced this pull request Jul 3, 2015

Merge pull request #224 from asergeyev/master

6a8b26e

Added function for lazy message reading per #222

miekg merged commit 6a8b26e into miekg:master Jul 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added function for lazy message reading per #222 #224

Added function for lazy message reading per #222 #224

asergeyev commented Jun 24, 2015

miekg commented Jun 24, 2015

miekg Jun 24, 2015

asergeyev Jun 25, 2015

miekg Jun 25, 2015

miekg commented Jun 24, 2015

asergeyev commented Jun 25, 2015

miekg commented Jun 26, 2015

asergeyev commented Jun 26, 2015

miekg commented Jun 26, 2015

asergeyev commented Jun 29, 2015

miekg commented Jun 29, 2015

asergeyev commented Jun 29, 2015

miekg Jun 30, 2015

asergeyev Jul 1, 2015

miekg Jul 1, 2015

miekg Jul 2, 2015

asergeyev commented Jul 1, 2015

asergeyev commented Jul 1, 2015

miekg commented Jul 2, 2015

asergeyev commented Jul 2, 2015

miekg commented Jul 2, 2015

miekg Jul 2, 2015

miekg Jul 2, 2015

asergeyev commented Jul 3, 2015

miekg commented Jul 3, 2015

Added function for lazy message reading per #222 #224

Added function for lazy message reading per #222 #224

Conversation

asergeyev commented Jun 24, 2015

miekg commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miekg commented Jun 24, 2015

asergeyev commented Jun 25, 2015

miekg commented Jun 26, 2015

asergeyev commented Jun 26, 2015

miekg commented Jun 26, 2015

asergeyev commented Jun 29, 2015

miekg commented Jun 29, 2015

asergeyev commented Jun 29, 2015

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

asergeyev commented Jul 1, 2015

asergeyev commented Jul 1, 2015

miekg commented Jul 2, 2015

asergeyev commented Jul 2, 2015

miekg commented Jul 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asergeyev commented Jul 3, 2015

miekg commented Jul 3, 2015