server: skip unnecessary sprintf which executes proto.Size()

[wilsonwang371]

From the profiling, I saw that when we call proto.Size(), it will unmarshal the data and return the size, which is costly. Similar to what we got in #12871

To avoid unnecessary proto.Size() call, I created this patch.

Some initial evaluation looks good. More data generating is in progress. It can increase the throughput by around 10% when we have small data size and a large number of requests.

@gyuho @ptabor

[codecov-commenter]

Codecov Report

Merging #13075 (4b58d64) into main (004081c) will increase coverage by 2.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #13075      +/-   ##
==========================================
+ Coverage   61.68%   64.48%   +2.80%     
==========================================
  Files         435      438       +3     
  Lines       34010    34057      +47     
==========================================
+ Hits        20978    21962     +984     
+ Misses      10953     9999     -954     
- Partials     2079     2096      +17     

Flag	Coverage Δ
all	64.48% <100.00%> (+2.80%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/etcdserver/util.go	72.72% <100.00%> (-19.06%)	⬇️
etcdctl/ctlv3/command/watch_command.go	5.29% <0.00%> (-37.65%)	⬇️
server/proxy/httpproxy/metrics.go	88.88% <0.00%> (-11.12%)	⬇️
server/proxy/httpproxy/director.go	55.71% <0.00%> (-10.01%)	⬇️
client/pkg/v3/testutil/testingtb.go	36.95% <0.00%> (-8.70%)	⬇️
server/proxy/httpproxy/reverse.go	47.70% <0.00%> (-8.26%)	⬇️
client/pkg/v3/testutil/testutil.go	35.29% <0.00%> (-5.89%)	⬇️
etcdctl/ctlv2/command/util.go	40.25% <0.00%> (-2.52%)	⬇️
server/mvcc/index.go	72.38% <0.00%> (-1.88%)	⬇️
... and 60 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 004081c...4b58d64. Read the comment docs.

[nekto0n]

I thought maybe it is worth adding some benchmarks? Something as simple as:

func BenchmarkWarnOfExpensiveRequestNoLog(b *testing.B) {
	m := &raftpb.Message{
		Type:       0,
		To:         0,
		From:       1,
		Term:       2,
		LogTerm:    3,
		Index:      0,
		Entries:    []raftpb.Entry{
			{
				Term:  0,
				Index: 0,
				Type:  0,
				Data:  make([]byte, 1024),
			},
		},
		Commit:     0,
		Snapshot:   raftpb.Snapshot{},
		Reject:     false,
		RejectHint: 0,
		Context:    nil,
	}
	err := errors.New("some error text")
	for n := 0; n < b.N; n++ {
		warnOfExpensiveRequest(testLogger, time.Second, time.Now(), nil, m, err)
	}
}

Gives:

benchcmp old.txt new.txt 
benchmark                                  old ns/op     new ns/op     delta
BenchmarkWarnOfExpensiveRequestNoLog-8     1005          69.3          -93.11%

and

benchstat old.txt new.txt 
name                           old time/op  new time/op  delta
WarnOfExpensiveRequestNoLog-8  1.00µs ± 0%  0.07µs ± 0%   ~     (p=1.000 n=1+1)

[gyuho]

@wilsonwang371 Thanks. Can we add the benchmark from @nekto0n as well?

[wilsonwang371]

This is the performance evaluation. We can see the best performance improvement can be around 10%. The worst case is around 4% degradation but I think this is not related to the new changes. and is within the performance variance.

[gyuho]

@wilsonwang371 Let's add that benchmark case for future reference, otherwise LGTM. Thanks.

[wilsonwang371]

@wilsonwang371 Let's add that benchmark case for future reference, otherwise LGTM. Thanks.

Just added the benchmark code.

[gyuho]

lgtm thanks.

[gyuho]

@wilsonwang371 Can you help with back porting to 3.5 as well? /cc @hexfusion

[wilsonwang371]

@wilsonwang371 Can you help with back porting to 3.5 as well? /cc @hexfusion

I posted backport pull request #13081