Ripley performance improvements #8

eugenepaniot · 2023-03-06T19:54:22Z

Transitioned to using https://github.com/valyala/fasthttp/ for the HTTP client;
Transitioned to using github.com/valyala/fastjson/ for fast json parsing;
Add custom Connection Diler to count TCP metrics (connections, bytes);
Add metrics server;
Add other metrics;
Add handleResult listener rather create goroutine;
Buffered channels;
Rewrite dummyweb server on fasthttp with reuseport;
Add store and print N results with the longest latency;
Improve error log reporting (connection errors, http errors 5xx);
Other UX improvements.

It is impossible to customise Host http header as fasthttp set it automaticaly from URI. To deal with it need to use fasthttp.HostClient with Addr instead. This change adds new extra parameter -target to set connection address.

georgemalamidis-lh

@eugenepaniot I haven't been able to finish the review. Sharing my comments so far. I currently can't run the tests because:

# command-line-arguments [command-line-arguments.test]
pkg/replay.go:136:16: undefined: unsafe.String
pkg/replay.go:136:30: undefined: unsafe.SliceData

georgemalamidis-lh · 2023-03-20T12:08:34Z

pkg/replay.go


 	return exitCode
 }
+
+func b2s(b []byte) string {
+	return unsafe.String(unsafe.SliceData(b), len(b))


Why not just return string(b)? And we don't need a function for it, we can just inline to string(b) in all the places we make this conversion.

Directly converting string(bytes) or []byte(str) will bring data duplication and poor performance, so in the pursuit of the ultimate performance scenario, need to use the “hack” way to achieve these two types of conversion. Like k8s does

Directly converting string(bytes) or []byte(str) will bring data duplication and poor performance, so in the pursuit of the ultimate performance scenario, need to use the “hack” way to achieve these two types of conversion. Like k8s does

Have we measured with and without the "hack"? What was the difference in performance?
What do you mean "data duplication", are we making more allocations and more GC as a result?

Right, it converts a byte to a string without memory allocation.
To be honest, I didn't verify it myself, but I come across this function's implementation in many Golang repos. My findings of the root cause of the issue led to discussion.

Just tested:

go test -bench=. goos: darwin goarch: amd64 pkg: github.com/loveholidays/ripley/tmp cpu: VirtualApple @ 2.50GHz BenchmarkTestJsonRequestString-10 20031897 52.29 ns/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5111 ns/op PASS ok github.com/loveholidays/ripley/tmp 1.980s

go test -run=. -bench=. -benchtime=5s -count 5 -benchmem -cpuprofile=cpu.out -memprofile=mem.out -trace=trace.out goos: darwin goarch: amd64 pkg: github.com/loveholidays/ripley/tmp cpu: VirtualApple @ 2.50GHz BenchmarkTestJsonRequestString-10 79934452 63.35 ns/op 288 B/op 1 allocs/op BenchmarkTestJsonRequestString-10 90362182 63.70 ns/op 288 B/op 1 allocs/op BenchmarkTestJsonRequestString-10 95226892 63.04 ns/op 288 B/op 1 allocs/op BenchmarkTestJsonRequestString-10 89002558 64.04 ns/op 288 B/op 1 allocs/op BenchmarkTestJsonRequestString-10 89922666 64.37 ns/op 288 B/op 1 allocs/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5158 ns/op 0 B/op 0 allocs/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5165 ns/op 0 B/op 0 allocs/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5161 ns/op 0 B/op 0 allocs/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5161 ns/op 0 B/op 0 allocs/op BenchmarkTestJsonRequestB2S-10 1000000000 0.5148 ns/op 0 B/op 0 allocs/op PASS ok github.com/loveholidays/ripley/tmp 36.985s

string memory:

b2s memory: no memory usage

b2s cpu:

string cpu:

georgemalamidis-lh · 2023-03-20T12:14:07Z

pkg/replay.go

+		if pacer.done {
+			break
+		}
+		waitGroupResults.Add(1)


I think this reads cleaner if we put it right before line 100 (results <- res)

waitGroupResults.Add is necessary there because the subsequent code sends to the results channel that runs waitGroupResults.Done function. If we move it after, an error may occur.

I also moved to the beginning - the if pacer.done condition to stop read requests as quickly as possible. However, I don't mind to move it back.

georgemalamidis-lh · 2023-03-20T12:18:30Z

pkg/metrics.go

+	"github.com/VictoriaMetrics/metrics"
+)
+
+var metricsRequestReceived = make(chan bool)


No global variables, please. Use the same pattern we use for the channels in https://github.com/loveholidays/ripley/blob/main/pkg/client.go#L34

The desire to use global was simply too strong to overcome.

georgemalamidis-lh · 2023-03-20T12:19:25Z

pkg/replay.go

-			}
-		}()
+	select {
+	case <-metricsRequestReceived:


I was confused as to where this came from and had to search for the global var in metrics.go. I've left a comment about it there.

pkg/result_longest_results.go

georgemalamidis-lh · 2023-03-20T12:26:42Z

pkg/replay.go

+	PrintStat           bool
+	MetricsServerEnable bool
+	MetricsServerAddr   string
+	NlongestPrint       bool


Can we combine NlongestPrint and NlongestResults into one flag, e.g. printNSlowest?

eugenepaniot · 2023-03-21T20:37:31Z

@eugenepaniot I haven't been able to finish the review. Sharing my comments so far. I currently can't run the tests because:
# command-line-arguments [command-line-arguments.test]
pkg/replay.go:136:16: undefined: unsafe.String
pkg/replay.go:136:30: undefined: unsafe.SliceData

The golang 1.20 is required as these function introduced in 1.20 version.

…ation

panic: runtime error: index out of range [0] with length 0 in waitDuration

georgemalamidis-lh · 2023-03-29T19:46:06Z

main.go

+
+	flag.StringVar(&opts.Pace, "pace", "10s@1", `[duration]@[rate], e.g. "1m@1 [email protected] 1h@2"`)
+	flag.BoolVar(&opts.Silent, "silent", false, "Suppress output")
+	flag.BoolVar(&opts.SilentHttpError, "silentHttpError", false, "Suppress HTTP errors (http codes 5xx) output")


When is this useful?

silentHttpError could be useful when need to do some bechmark testing exclusively or in situations when it's acceptable to overlook any HTTP errors.

sonarqubecloud · 2024-01-26T17:25:06Z

Quality Gate failed

Failed conditions

2 Security Hotspots

See analysis details on SonarCloud

eugenepaniot added 7 commits March 6, 2023 14:42

Use fasthttp.Client

3164dd7

Use fasthttp.Client

5c1e826

Use handleResult

bc13f29

Add minimum wait duration

c75b37d

Update dockerimage

ff69c7e

Always time.Sleep

6cf879a

Rewrite dummyweb.go using fasthttp

89c0b69

eugenepaniot requested a review from georgemalamidis-lh March 6, 2023 20:02

eugenepaniot added 22 commits March 7, 2023 16:29

Add TestFasthttpRequest

9519a00

Use fasthttp.HostClient instead of fasthttp.Client

f2d2014

It is impossible to customise Host http header as fasthttp set it automaticaly from URI. To deal with it need to use fasthttp.HostClient with Addr instead. This change adds new extra parameter -target to set connection address.

Reinvent http client

5f4ce0d

Reinvent http client

aaa33fd

Update metrics

c3c0ae2

Add pacer_phases metric

a0012f0

Add linegen to generate data for ripley

ed64d4e

Update

17991ed

Use memory pointer in unmarshalRequest

5254c13

Add metricsRequestReceived channel to wait for the last metrics scrape

14b823a

Add nLongestResults

c4e72c3

Thread-safe clientsPool LoadOrStore

15650fe

Thread-safe clientsPool LoadOrStore

a157732

Add SilentHttpError option

f8b2f26

json.Marshal to result.toJson

c86c63a

update go.mod

c875e6a

Add Result.Response

3547f15

Use go 1.20

313b98e

Use httpResp.SkipBody = true

583fc8a

Better error handling

b7d26f0

Optimised b2s func

9880fa6

update deps

8424b7f

eugenepaniot added 2 commits March 16, 2023 15:21

Fix waitGroupResults race

ebd0cc8

Rollback httpResp.SkipBody as some strange behaviour occures

f3c5427

georgemalamidis-lh reviewed Mar 20, 2023

View reviewed changes

eugenepaniot added 8 commits March 22, 2023 20:32

Use Slices for SlowestResults rather Heap

d16fbf9

Do not use global vars for metric recv channel

f22cae9

printNSlowest over NlongestPrint and NlongestResults

6b59a2a

copy Result

ce23a48

store

8687e72

panic: runtime error: index out of range [0] with length 0 in waitDur…

f026f19

…ation

update new pacer call

c9d382e

Use synchronization in waitDuration and onPhaseElapsed

f1e5bd0

panic: runtime error: index out of range [0] with length 0 in waitDuration

eugenepaniot force-pushed the benchmark-ep branch from 07db321 to f1e5bd0 Compare March 24, 2023 00:07

Print metrics and slowest results by signal

24c0aa3

eugenepaniot requested a review from georgemalamidis-lh March 27, 2023 13:09

georgemalamidis-lh reviewed Mar 29, 2023

View reviewed changes

eugenepaniot mentioned this pull request Jul 15, 2023

Minor fixes #9

Closed

Merge branch 'main' into benchmark-ep

50933a6

This was referenced Jul 25, 2024

DV Quick Code Changes #14

Closed

dyn workers #13

Closed

georgemalamidis-lh closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ripley performance improvements #8

Ripley performance improvements #8

eugenepaniot commented Mar 6, 2023 •

edited

Loading

georgemalamidis-lh left a comment

georgemalamidis-lh Mar 20, 2023

eugenepaniot Mar 21, 2023

georgemalamidis-lh Mar 21, 2023

eugenepaniot Mar 21, 2023

eugenepaniot Mar 21, 2023

eugenepaniot Mar 22, 2023

georgemalamidis-lh Mar 20, 2023 •

edited

Loading

eugenepaniot Mar 21, 2023 •

edited

Loading

georgemalamidis-lh Mar 20, 2023

eugenepaniot Mar 21, 2023

eugenepaniot Mar 22, 2023

georgemalamidis-lh Mar 20, 2023

eugenepaniot Mar 22, 2023

georgemalamidis-lh Mar 20, 2023

eugenepaniot Mar 22, 2023

eugenepaniot commented Mar 21, 2023

georgemalamidis-lh Mar 29, 2023

eugenepaniot Mar 30, 2023

sonarqubecloud bot commented Jan 26, 2024

Ripley performance improvements #8

Ripley performance improvements #8

Conversation

eugenepaniot commented Mar 6, 2023 • edited Loading

georgemalamidis-lh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgemalamidis-lh Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

eugenepaniot Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenepaniot commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 26, 2024

Quality Gate failed

eugenepaniot commented Mar 6, 2023 •

edited

Loading

georgemalamidis-lh Mar 20, 2023 •

edited

Loading

eugenepaniot Mar 21, 2023 •

edited

Loading