{writer,info}: fix races

[gbbr]

No description provided.

[LotharSee]

This is getting called every time we sample a trace. Is there any way to avoid having this lock in the hotpath? Which variable is subject to a race condition?

[gbbr]

I'd guess both of them are, right? The issue is calling SetSignatureCoefficients at the same time. AFAIK the performance loss is not really significant when there is not much contention. The read lock is simply an atomic.LoadInt* under the hood.

[LotharSee]

Yeah I was curious about how it compares vs using atomic.LoadInt directly on signatureScoreFactor and signatureScoreSlope. I guess it should be very close.

One or the other, what about systematically introduce a benchmark whenever we add a lock somewhere, to illustrate the resulting trace-off? (the same way we introduce a test when we fix a bug).

[gbbr]

atomic.LoadInt wouldn't work here, because it's a float. But I'm happy to introduce a benchmark. Although, what would the benchmark do? Run AdjustScoring every 10*time.Second and then in parallel GetCountScore (this is what happens in the agent)? Or were you thinking simply benchmarking calling GetCountScore with and without locks?

[gbbr]

I guess the latter probably makes most sense. I will add that.

[gbbr]

Ok, so the results are in:

name                          old time/op  new time/op  delta
BackendScoreToSamplerScore-8  16.3ns ± 0%  48.6ns ± 0%  +198.16%

Using this benchmark:

func BenchmarkBackendScoreToSamplerScore(b *testing.B) {
	s := newSampler(1.0, 10)
	for i := 0; i < b.N; i++ {
		s.backendScoreToSamplerScore(10)
	}
}

It's not looking too good. Do you have any ideas? I'm not sure if it's ok to leave this race here. Or?

[LotharSee]

Could the cost of a lock be actually much more than an atomic? Maybe that's the defer?

We can try to implement an atomic float (just copy what's in https://github.com/uber-go/atomic/blob/master/atomic.go#L267 ), use it on signatureScoreFactor and signatureScoreSlope and compare?

Also, all of that is to make it "the right way". In absolute that duration might not be that much compared to all the stuff on-going when doing sampling. So I wouldn't worry too too much.

[gbbr]

Nice ideas!

With no defer:

name                          old time/op  new time/op  delta
BackendScoreToSamplerScore-8  16.3ns ± 0%  25.1ns ± 0%  +53.99%

With Uber's atomic Float64:

name                          old time/op  new time/op  delta
BackendScoreToSamplerScore-8  16.3ns ± 0%  17.5ns ± 0%  +7.36%

I'll go ahead an add the Uber version.

[LotharSee]

Nice, that's great learnings! defer is big.

[gbbr]

Indeed!

[gbbr]

@LotharSee should be super close to what it was now, without the race:

name                          old time/op  new time/op  delta
BackendScoreToSamplerScore-8  16.3ns ± 0%  17.5ns ± 0%  +7.36%

PTAL