several improvements to your great app

[robaho]

The changes are:

doesn't report errors to console to make it hard to read. creates a map of errors and prints that at the end.

adds a histogram for timings

no longer sets GOMAXPROCS directly. I think this was incorrect. Still, you can change it usings -cpus or GOMAXPROCS environment variable.

updates Go to latest version, 1.22

update Go modules

Running 30s test @ http://imac:8080/plaintext
  64 goroutine(s) running concurrently
2337304 requests in 29.958010816s, 263.03MB read
Requests/sec:		78019.33
Transfer/sec:		8.78MB
Fastest Request:	311µs
Avg Req Time:		819µs
Slowest Request:	27.064ms
Number of Errors:	0
Error Counts:		map[]
10%:			458µs
50%:			515µs
75%:			533µs
99%:			543µs
99.9%:			543µs
99.9999%:		543µs
99.99999%:		543µs
stddev:			301µs

[tsliwowicz]

Thanks @robaho ! Nice cleanup (many of these things like hdrhistograms were not available when I wrote this).
I will review and test.
Regarding GOMAXPROCS - it was an issue - CPUs were underutilized otherwise. I'm not sure but maybe this improved. In any case, I think we should consider keeping this as default and overriding though the param as needed.
Mind making the code changes and resolve the conflicts so I would be able to merge?

[robaho]

Thanks @robaho ! Nice cleanup (many of these things like hdrhistograms were not available when I wrote this). I will review and test. Regarding GOMAXPROCS - it was an issue - CPUs were underutilized otherwise. I'm not sure but maybe this improved. In any case, I think we should consider keeping this as default and overriding though the param as needed. Mind making the code changes and resolve the conflicts so I would be able to merge?

Hi. Glad to help.

I am 99.9999% certain you do not want to set GOMAXPROCS to the number of Go routines. Go uses netpoller for network IO in order to efficiently handle the network requests with a minimum of platform/hardware threads. This may have been needed in very early Go versions, but shouldn't be the case now.

For instance, using the same client/server setup:

Using default GOMAXPROCS:

robertengels@macmini go-wrk % ./go-wrk -c 1000 -d 30 -T 10000 http://imac:8080/plaintext 
Running 30s test @ http://imac:8080/plaintext
  1000 goroutine(s) running concurrently
4332783 requests in 29.277238801s, 458.66MB read
Requests/sec:		147991.52
Transfer/sec:		15.67MB
Overall Requests/sec:	141803.01
Overall Transfer/sec:	15.01MB
Fastest Request:	91µs
Avg Req Time:		6.756ms
Slowest Request:	816.671ms
Number of Errors:	2182
Error Counts:		connection reset by peer=2147,net/http: timeout awaiting response headers=35
10%:			1.68ms
50%:			2.395ms
75%:			2.482ms
99%:			2.536ms
99.9%:			2.537ms
99.9999%:		2.538ms
99.99999%:		2.538ms
stddev:			27.955ms

Setting GOMAXPROCS equal to the number of Go routines:

robertengels@macmini go-wrk % ./go-wrk -c 1000 -d 30 -cpus 1000 -T 10000 http://imac:8080/plaintext
Running 30s test @ http://imac:8080/plaintext
  1000 goroutine(s) running concurrently
2014051 requests in 29.315755987s, 213.20MB read
Requests/sec:		68702.00
Transfer/sec:		7.27MB
Overall Requests/sec:	65266.19
Overall Transfer/sec:	6.91MB
Fastest Request:	59µs
Avg Req Time:		14.555ms
Slowest Request:	2.528767s
Number of Errors:	1471
Error Counts:		connection reset by peer=1470,broken pipe=1
10%:			121µs
50%:			160µs
75%:			173µs
99%:			183µs
99.9%:			183µs
99.9999%:		183µs
99.99999%:		183µs
stddev:			68.391ms

The problem is the thread scheduling overhead it creates - so the throughput and average times drop by more than 50% in this case. It only gets worse the higher you set GOMAXPROCS.

I did send an email to go-nuts asking about this. I believe it is a different story if you are performing disk IO.

I am more than happy to make it mergable - the only reason is that I am using later versions of the packages.

[tsliwowicz]

ok. Thanks for asking them. I saw the answer.
Yes - when there's disk access you'd want to do that, but I agree that in this case it's not required. And looking at our results, it's apparent.
I concur then. Lets go with your fix then.
Mind doing the other adjustments, I'll give it a final review and merge?

[robaho]

ok. Thanks for asking them. I saw the answer. Yes - when there's disk access you'd want to do that, but I agree that in this case it's not required. And looking at our results, it's apparent. I concur then. Lets go with your fix then. Mind doing the other adjustments, I'll give it a final review and merge?

What other adjustments? I updated the PR for the other changes - and it says it can be automatically merged.

[tsliwowicz]

Thanks @robaho !

[robaho]

Thanks @robaho !

You’re welcome!