Fixes from @albrow's Wasm review

[nhooyr]

@albrow

See #142 (review)

[nhooyr]

Some discussion at 82db14a#r35201593

[albrow]

While this approach is fast, I think it doesn't allow the GC to free the associated memory since the underlying array still contains every element. This is different from bytes.Buffer which does attempt to recover/free memory when possible. See https://golang.org/src/bytes/buffer.go?s=9451:9501#L286.

[nhooyr]

GC should be fine as when a new message arrives, the slice will be appended to and the old slice thrown away. I'll even nil it out immediately to make sure GC knows the message event isn't necessary anymore.

[nhooyr]

This is different from bytes.Buffer which does attempt to recover/free memory when possible. See https://golang.org/src/bytes/buffer.go?s=9451:9501#L286.

Not sure what you're referring to here. *bytes.Buffer never attempts to free/recover memory afaik.

[albrow]

I was referring to these lines in bytes.Buffer.Read:

if b.empty() {
	// Buffer is empty, reset to recover space.
	b.Reset()
	if len(p) == 0 {
		return 0, nil
	}
	return 0, io.EOF
}

I suppose "free" is not the right word since the memory is held and reused by the buffer. b.Reset is called in few other places in the implementation too.

What I meant to draw attention to is that just because the old slice is thrown away doesn't mean the underlying array is. Just looking at the line c.readBuf = c.readBuf[1:] made me concerned that we might be holding on to every element in the underlying array indefinitely. However, you're right that this is not the case because of the use of append. The memory associated with the underlying array will be freed if append causes the slice to grow (at which point the remaining data is copied to a new underlying array). This probably happens fairly often so it should be fine.

It's still true that bytes.Buffer has some memory optimizations (e.g., calling Reset when possible in order to reuse memory) that aren't present here but it might not be a big deal.

[nhooyr]

Have fixed this anyway in my latest commit. c.readBuf is now fixed size at whatever the limit is set (right now 32) and elements are shifted around instead of slicing the array to avoid allocations.

[albrow]

Hmm.. this version requires fewer memory allocations but it also calls copy on every read. I'm curious if this has any impact on performance? It might be fine.

[nhooyr]

I don't expect it to be a problem. Copying data is very fast, especially since we're only copying pointers to the data.

[nhooyr]

Fixed all of the issues you've pointed out @albrow. Please review again.