Leave overflow logic to go-msgio #1910

tilgovi · 2015-10-29T02:19:07Z

It seems as though go-msgio now implements the message length limit
(currently 8MB), so the logic in secio reader can be simplified.

This is my first go code. I could be wildly wrong about many things.

I tried to run make test and got failures, but there are no tests for this module, it seems, so I don't know if these are failures that I've caused.

GitCop · 2015-10-29T02:19:08Z

There were the following issues with your Pull Request

Commit: e698aa2
- Invalid signoff. Commit message must end with
  License: MIT
  Signed-off-by: .* <.*>

Guidelines and a script are available to help. Your feedback on GitCop is welcome on this issue.

This message was auto-generated by https://gitcop.com

tilgovi · 2015-10-29T02:22:08Z

I'm able to run the tests locally. Things are failing. I'll see what's up.

jbenet · 2015-10-29T08:53:34Z

I believe we still need to have a buffer here, for the ETM behavior to work correctly. (also will want to be able to tune it here, separately from having to change go-msgio)

tilgovi · 2015-10-29T18:51:05Z

There still is a buffer here and there can be tuning added, but right now what's here is just redundant. The exact same check and error is thrown by go-msgio.

tilgovi · 2015-10-29T18:52:42Z

Unless I misunderstand, but it looks like one message from the msgio is one payload+mac, and the buffer here is just because the reader might request less than the full payload and, regardless of the chunking and chunk digests, it seems like the reader is meant to receive a continuous stream.

GitCop · 2015-10-29T22:35:44Z

There were the following issues with your Pull Request

Commit: d6474c3
- Invalid signoff. Commit message must end with
  License: MIT
  Signed-off-by: .* <.*>

Guidelines and a script are available to help. Your feedback on GitCop is welcome on this issue.

This message was auto-generated by https://gitcop.com

tilgovi · 2015-10-29T22:51:51Z

p2p/crypto/secio/rw.go

 func (r *etmReader) Read(buf []byte) (int, error) {
 	r.Lock()
 	defer r.Unlock()

-	// first, check if we have anything in the buffer
-	copied := r.drainBuf(buf)
-	buf = buf[copied:]


I think this was meant to deal with the case where buf is smaller than r.buf but I think it should read buf = buf[:copied] (all the bytes up to copied, not all the bytes after).

no i believe this is right as is -- (this seems to be way more complicated than it should be, though).

buf here is the user passed in buf which will receive the data

r.drainBuf(buf) will copy into buf all the stuff in r.buf that fits into buf.

it also updates r.buf to be r.buf[n:] (i.e. already copied buf[0:n] into buf)

copied is the number of bytes copied into buf. this may be a number smaller than len(buf), and if so, we want to set buf to be buf[copied:] so that further copies into buf only fill up the remaining (unused) bytes.

below, we copy more into buf, and in that case we only write into the original buf[copied:], because that's what hasn't been written to.

all of this is useless anyway because exit if copied > 0, which seems to have been added later, as a short circuit for simplicity.

also, wow this code. @jbenet needs to write cleaner crap.

Ah, thanks for the explanation. This is me being a go newbie, despite being an experience C programmer. I was thinking that somehow buf was manipulating a reference and buf = buf[copied:] was changing the result that the caller sees.

tilgovi · 2015-10-29T22:52:16Z

All fixed. Even if you don't want to remove this overflow logic, please see my comment in the changes: I think there is a bug.

tilgovi · 2015-10-29T23:23:09Z

Well, this is ready to be either closed or merged, I think. Your call :).

jbenet · 2015-10-29T23:29:44Z

I really like the simplicity, and this seems right to me. (tests seem to pass too, so should be right).

the only thing i wonder about is whether the buffer should be held here or left to go-msgio. msgio may change again, not clear. and we may want to make this buffer configurable, not sure.

@whyrusleeping is going to be a much better person to CR this than me.

jbenet · 2015-10-29T23:33:20Z

Oh, now i recall. the thing about using go-msgio Read() instead of ReadMsg() is that with Read() we keep one buffer allocated at all times (the etmReader's), per encrypted channel. instead, with ReadMsg() we allocate one buffer per new message (minus the pool thing, but im not convinced that thing works very well). the gc pressure is larger and thus may be lower perf. (right @whyrusleeping ?) and this is one of the few places where that actually matters

tilgovi · 2015-10-29T23:39:41Z

Oh, I see. Since the caller is passing in a buffer, we can ask msgio to fill it with the io.ReadFull call. Otherwise, r.msg.ReadMsg() is going to allocate a buffer internally.

tilgovi · 2015-10-29T23:41:37Z

The msgio.reader uses a buffer pool, though. :-/

tilgovi · 2015-10-29T23:45:04Z

Although, I never release the buffer back to the pool.

@jbenet if there's still desire to make all of this more streaming somehow, I'm happy to hack at that instead.

whyrusleeping · 2015-10-30T07:49:02Z

wow, so i've never looked at this etm code before.

At first glance, this looks like the right idea to me. although one thing to note: you will need to keep track of the original buffer handed to you from msgio, and call r.ReleaseMsg on it when its empty, memory pressure here is important (and we apparently havent been releasing the messages here at all 👎 )

I'll play with this a little more in the morning after you make the ReleaseMsg change.

jbenet · 2015-10-30T15:12:37Z

We haven't needed to release the messages because we used Read with a single buffer, no more allocations.

—
Sent from Mailbox

On Fri, Oct 30, 2015 at 3:49 AM, Jeromy Johnson notifications@github.com
wrote:

wow, so i've never looked at this etm code before.
At first glance, this looks like the right idea to me. although one thing to note: you will need to keep track of the original buffer handed to you from msgio, and call r.ReleaseMsg on it when its empty, memory pressure here is important (and we apparently havent been releasing the messages here at all 👎 )

I'll play with this a little more in the morning after you make the ReleaseMsg change.

Reply to this email directly or view it on GitHub:
#1910 (comment)

whyrusleeping · 2015-10-30T16:37:39Z

@jbenet oooooh, right.

tilgovi · 2015-10-30T17:54:22Z

Thanks, you two. Closing this. Good first exercise, though :).

whyrusleeping · 2015-10-30T17:57:46Z

@tilgovi i think theres still something there worth pursuing in this PR. your PR would lower memory pressure (as long as you added the release call), and simplify this part of the code quite a bit.

tilgovi · 2015-10-30T18:08:49Z

Re-opened. Definitely agree it simplifies, but I don't see how it alleviates any memory pressure. As @jbenet pointed out, the user is passing in a buffer here. Although, in the case that the user buffer is too small for the message, we're allocating a new buffer here anyway. That would be better to do in msgio since it's got a buffer pool.

If you think it's helpful, I'll add the release back to the pool. Anything else you would like me to change?

jbenet · 2015-10-30T18:19:52Z

@whyrusleeping i believe the old way is much better for memory. It's a larger constant allocation, but it doesn't cause a bunch of sequential allocs of varying size which will undoubtedly lead to fragmentation. (worse mem and cpu utilization).

Anyway, the simplification and cleanup is worth pursuing

tilgovi · 2015-10-30T18:23:54Z

I'll just let it bake in my head over the weekend and see if there's anything I think is worthwhile on Monday.

whyrusleeping · 2015-10-30T18:24:57Z

@jbenet the memory pools are rather efficient. they work quite well

tilgovi · 2015-11-03T00:26:21Z

I've rewritten this but I'm sure I have a bug somewhere because the tests are failing.

The current implementation aims to read directly into the output buffer when it is sufficiently large and to let msgio allocate from its pool otherwise, draining from and releasing that when it's been used up.

This should be the best of both worlds: zero-copy if the output buffer is large enough and buffer pool otherwise.

tilgovi · 2015-11-03T00:26:37Z

I'll let you know when I work out the kinks if no one spots it before I do.

tilgovi · 2015-11-04T22:46:01Z

There you go.

jbenet · 2015-11-10T08:23:51Z

this LGTM. thanks @tilgovi

@whyrusleeping CR?

whyrusleeping · 2016-01-29T17:38:27Z

@tilgovi sorry for taking so long on this one, could you rebase it on latest master?

go-msgio implements the message length limit (currently 8MB), so the secio reader can get away without re-implementing this logic. Instead, the reader lets go-msgio allocate a buffer when the output buffer is too small to hold the incoming message. This buffer is kept and drained into the output buffer(s) of Read() calls and released back to the msgio instance once it is fully drained. License: MIT Signed-off-by: Randall Leeds <randall@bleeds.info>

tilgovi · 2016-01-29T18:10:43Z

Done. Not all the CI passed, but I can't see how it's related to the change and they may be spurious. I don't know the state of the test suite.

whyrusleeping · 2016-01-29T19:51:40Z

Yeah, the test failures arent related. The one problem we have now right now is #2256 all this code is being moved out of the repo into libp2p. I can handle moving this commit over there if you'd like

tilgovi · 2016-01-30T01:46:25Z

Moved to libp2p/go-libp2p#13

jbenet added the backlog label Oct 29, 2015

tilgovi force-pushed the cleanup-secio-reader-overflow branch from e698aa2 to d6474c3 Compare October 29, 2015 22:35

tilgovi force-pushed the cleanup-secio-reader-overflow branch from d6474c3 to 1c7e286 Compare October 29, 2015 22:38

tilgovi reviewed Oct 29, 2015
View reviewed changes

tilgovi force-pushed the cleanup-secio-reader-overflow branch from 1c7e286 to 6c95889 Compare October 29, 2015 23:22

tilgovi closed this Oct 30, 2015

jbenet removed the backlog label Oct 30, 2015

tilgovi deleted the cleanup-secio-reader-overflow branch October 30, 2015 17:54

tilgovi restored the cleanup-secio-reader-overflow branch October 30, 2015 18:06

tilgovi reopened this Oct 30, 2015

jbenet added the backlog label Oct 30, 2015

tilgovi force-pushed the cleanup-secio-reader-overflow branch 3 times, most recently from 51808c7 to 42697e6 Compare November 2, 2015 23:31

tilgovi force-pushed the cleanup-secio-reader-overflow branch from 42697e6 to f9684c6 Compare November 4, 2015 21:58

ghost added topic/encryption Topic encryption RFCR labels Dec 22, 2015

daviddias removed the backlog label Jan 2, 2016

tilgovi force-pushed the cleanup-secio-reader-overflow branch from f9684c6 to 6eedb6c Compare January 29, 2016 17:52

tilgovi closed this Jan 30, 2016

tilgovi deleted the cleanup-secio-reader-overflow branch January 30, 2016 01:46

Leave overflow logic to go-msgio #1910

Leave overflow logic to go-msgio #1910

Conversation

tilgovi commented Oct 29, 2015

GitCop commented Oct 29, 2015

tilgovi commented Oct 29, 2015

jbenet commented Oct 29, 2015

tilgovi commented Oct 29, 2015

tilgovi commented Oct 29, 2015

GitCop commented Oct 29, 2015

tilgovi Oct 29, 2015

Choose a reason for hiding this comment

jbenet Oct 29, 2015

Choose a reason for hiding this comment

jbenet Oct 29, 2015

Choose a reason for hiding this comment

tilgovi Oct 29, 2015

Choose a reason for hiding this comment

tilgovi commented Oct 29, 2015

tilgovi commented Oct 29, 2015

jbenet commented Oct 29, 2015

jbenet commented Oct 29, 2015

tilgovi commented Oct 29, 2015

tilgovi commented Oct 29, 2015

tilgovi commented Oct 29, 2015

whyrusleeping commented Oct 30, 2015

jbenet commented Oct 30, 2015

I'll play with this a little more in the morning after you make the ReleaseMsg change.

whyrusleeping commented Oct 30, 2015

tilgovi commented Oct 30, 2015

whyrusleeping commented Oct 30, 2015

tilgovi commented Oct 30, 2015

jbenet commented Oct 30, 2015

tilgovi commented Oct 30, 2015

whyrusleeping commented Oct 30, 2015

tilgovi commented Nov 3, 2015

tilgovi commented Nov 3, 2015

tilgovi commented Nov 4, 2015

jbenet commented Nov 10, 2015

whyrusleeping commented Jan 29, 2016

tilgovi commented Jan 29, 2016

whyrusleeping commented Jan 29, 2016

tilgovi commented Jan 30, 2016

I'll play with this a little more in the morning after you make the `ReleaseMsg` change.