gnet-io / gnet-benchmarks

Benchmark test of gnet
MIT License
34 stars 12 forks source link

some issues in this project may cause misleading information #6

Closed joway closed 3 years ago

joway commented 3 years ago

Hi, thanks for providing a benchmark project between gnet and netpoll. But it seems that the project has some critical issues which may cause misunderstanding.

I list the issues as follows, let me know if there is anything I missed.

Incorrect use of netpoll

Forget to release buffer in netpoll-echo-server which cause huge memory usage and make it cannot reuse the memory:

func handler(ctx context.Context, connection netpoll.Connection) error {
    reader := connection.Reader()
    buf, err := reader.Next(reader.Len())
        // You should release reader buffer after flush finished:
        // defer reader.Release()

        _, err = connection.Writer().WriteBinary(buf)
    return connection.Writer().Flush()
}

You will see a huge performance improvement when you fixed it.

Unfair copying

When using gnet, you just assign the input buffer to the output buffer without any copy:

func (es *echoServer) React(frame []byte, c gnet.Conn) (out []byte, action gnet.Action) {
    // Echo synchronously.
    out = frame
}

But for net-echo-server you do too much copy:

inBuf := make([]byte, 4*1024)
outBuf := bytes.NewBuffer(make([]byte, 0, 4*1024))

As to the netpoll, since it assuming that the input and output buffer cannot be the same buffer in production, so it uses different buffers and copies them internally.

I think it should have a declaration of this critical difference if you want to benchmark them all together.

Different design goals

netpoll is designed for RPC use cases so it will create/reuse a goroutine for every handler function which will have some earning in production but cause overhead in the ECHO benchmark.

It's ok if this benchmark project only wants to consider the ECHO/Redis-like use cases, but it should be better to point out this key difference at the beginning.

We know it's hard to do an absolute FAIR benchmark since the design goals of network packages are different, but still want to make the benchmark have the reference value for the people who want to make choice for their own use case. So we also opensourced our own benchmark repo in netpoll-benchmark which more focus on the RPC use case benchmark.

It's glad to see have more benchmark in this field. Also, it welcomes to raise your questions if you think there have any points for improvement in our benchmark.

panjf2000 commented 3 years ago

Incorrect use of netpoll

I wrote that netpoll example based on netpoll doc and I believe the doc didn't mention anything about reader.Release() back then.

I will update the netpoll example according to your suggestion later, though, I wouldn't say that example in your stale doc is a good example.

Unfair copying

The original net echo example is here, and surprisingly, the benchmark results didn't make a noticeable change before and after the modification, I think it's because: https://github.com/gnet-io/gnet-benchmarks/blob/dbe59e54f2ba76a1da24aae255d869bf05e6a7bc/net-echo-server/main.go#L47-L50 this branch is rarely hit.

As for the case of gnet, that's the best usage for a echo server. But I guess I can update it after I make an inspection tour to the source code of netpoll, figuring out how it copies the bytes between the reader and writer buffers. OTOH, I believe that the examples of net and gnet have the same number of copies if you take a deep look at the source code of net and gnet.

Different design goals

I'll second your statement about gnet and netpoll being applicable to different scenarios, and it is indeed unlikely to have a perfectly fair benchmark that satisfies everyone, I believe both of us can continue to improve the benchmarks, no way to be perfect, but keep getting closer.