jaegertracing / jaeger-clickhouse

Jaeger ClickHouse storage plugin implementation
Apache License 2.0
236 stars 50 forks source link

Refactored storage.SpanWriter #50

Closed EinKrebs closed 2 years ago

chhetripradeep commented 2 years ago

@EinKrebs I am not sure what was the motivation behind this PR. I thought that strings.Builder is faster than fmt.Sprintf

func BenchmarkStringsBuilder(b *testing.B) {
        buf := &strings.Builder{}
        for i := 0; i < b.N; i++ {
                buf.Reset()
                buf.WriteString("key")
                buf.WriteByte('=')
                buf.WriteString("value")
        }
}

func BenchmarkFmtSprintf(b *testing.B) {
        for i := 0; i < b.N; i++ {
                fmt.Sprintf("%s=%s", "key", "value")
        }
}
❯ go test -bench=.
BenchmarkStringsBuilder-12      26100945                44.33 ns/op
BenchmarkFmtSprintf-12          13052930                90.47 ns/op
bobrik commented 2 years ago

I initially started with fmt.Sprintf, but ended up with strings.Builder precisely because of what @chhetripradeep noted.

EinKrebs commented 2 years ago

@chhetripradeep @bobrik thank you for advice, gonna revert this.

EinKrebs commented 2 years ago

@chhetripradeep @bobrik @pavolloffay I got interesting results testing also strings concatenation.

func BenchmarkStringsBuilder(b *testing.B) {
    buf := &strings.Builder{}
    tags := generateRandomKeyValues(b.N)
    b.StartTimer()
    for i := 0; i < b.N; i++ {
        buf.Reset()
        buf.WriteString(tags[i].Key)
        buf.WriteByte('=')
        buf.WriteString(tags[i].AsString())
        _ = buf.String()
    }
}

func BenchmarkFmtSprintf(b *testing.B) {
    tags := generateRandomKeyValues(b.N)
    b.StartTimer()
    for i := 0; i < b.N; i++ {
        _ = fmt.Sprintf("%s=%s", tags[i].Key, tags[i].AsString())
    }
}

func BenchmarkPlus(b *testing.B) {
    tags := generateRandomKeyValues(b.N)
    b.StartTimer()
    for i := 0; i < b.N; i++ {
        _ = tags[i].Key + "=" + tags[i].AsString()
    }
}

There are the results

BenchmarkStringsBuilder
BenchmarkStringsBuilder-12       3488994           328.0 ns/op
BenchmarkFmtSprintf
BenchmarkFmtSprintf-12           2865664           407.5 ns/op
BenchmarkPlus
BenchmarkPlus-12                 3997849           298.3 ns/op
EinKrebs commented 2 years ago

I guess I'll use concatenation after all.

pavolloffay commented 2 years ago

concatenation seems better to me for this use-case.