Skip to content

Slice conn with faster write and faster read #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nhooyr opened this issue Apr 14, 2019 · 6 comments
Closed

Slice conn with faster write and faster read #54

nhooyr opened this issue Apr 14, 2019 · 6 comments

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Apr 14, 2019

Should we add a code path for fast reads and writes without allocating a reader or writer?

like:

type frame struct {
	opcode opcode
	p      []byte
}

func (c *Conn) Write2(ctx context.Context, dataType DataType, msg []byte) error {
	err := c.write2(ctx, dataType, msg)
	if err != nil {
		return xerrors.Errorf("failed to write message: %w", err)
	}
	return nil
}

func (c *Conn) write2(ctx context.Context, dataType DataType, msg []byte) error {
	select {
	case <-c.closed:
		return c.closeErr
	case <-ctx.Done():
		return ctx.Err()
	case c.write <- frame{
		opcode: opcode(dataType),
		p:      msg,
	}:
		select {
		case <-c.closed:
			return c.closeErr
		case <-ctx.Done():
			return ctx.Err()
		case <-c.writeDone:
			return nil
		}
	}
}

func (c *Conn) Read2(ctx context.Context, msg []byte) (DataType, int, error) {
	dataType, n, err := c.read2(ctx, msg)
	if err != nil {
		return 0, 0, xerrors.Errorf("failed to read message: %w", err)
	}
	return dataType, n, nil

}

func (c *Conn) read2(ctx context.Context, msg []byte) (DataType, int, error) {
	select {
	case <-c.closed:
		return 0, 0, c.closeErr
	case <-ctx.Done():
		return 0, 0, xerrors.Errorf("failed to read: %w", ctx.Err())
	case c.read <- msg:
		select {
		case <-c.closed:
			return 0, 0, c.closeErr
		case <-ctx.Done():
			return 0, 0, xerrors.Errorf("failed to read: %w", ctx.Err())
		case r := <-c.readDone:
			return r.dataType, r.n, nil
		}
	}
}

Would actually be a wrapper around the Conn like JSONConn though.

@nhooyr nhooyr added this to the v1.1.0 milestone Apr 14, 2019
@nhooyr nhooyr mentioned this issue Apr 14, 2019
3 tasks
@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2019

Also see golang/go#18152 (comment)

I don't think the writer allocation for every write is a big deal.

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 14, 2019

The major issue I see with the writer is you have to close it so every data msg greater than the buffer size of 4096 bytes will be sent with three writes on the conn instead of two.

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 15, 2019

I think the move is to remove the allocations altogether and just use a value type on the Conn and put the read/write state there.

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 15, 2019

That definitely removes the need for the Read method but the Write method makes sense to avoid the extra fin frame being written but I doubt anyone will ever notice that with the overhead of everything else in the stack so I'm good with the API as is.

@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 15, 2019

Definitely should benchmark this at some point with big payloads.

@nhooyr nhooyr reopened this Apr 15, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Apr 16, 2019

So I benched this and it turns out there is no latency difference but the extra syscall adds 5000ns which is 5µs which isn't really that much at all so I don't think its a big deal. Can revisit if someone complains about performance but this isn't really going to show up if you're writing messages larger than 4096 bytes and I can always increase the buffer size to something like 32kb which would be good enough for pretty much everyone. I don't want to add a special case API unless absolutely necessary.

@nhooyr nhooyr closed this as completed Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant