Add CachedReporter interface that supports pre allocation. #22

Raynos · 2017-01-20T01:30:22Z

This PR adds a new method NewCachedRootScope and a new
CachedReporter interface.

This CachedReporter interface allows for onetime allocation
of counters, gauges & timers. This allows for the CachedReporter
to cached the tags map and do pre-computation.

The actual runtime interface after all the scopes & metrics are
allocated at startup just involves ReportCount(int64) and friends.

This enables a reporter backend to implement a more involved BUT
more performant interface.

I've got a benchmark using an internal reporter where I was measuring the
overhead of adding 3 timers to a HTTP server.

master, 3 tally.Timers per request ~= 27% cpu overhead & 50 allocs per timer
this branch, 3 tally.Timers per request ~= 11% cpu overhead & 25 allocs per timer

This is a 2x improvement and allows us to have more timers in our
applications.

r: @robskillington @martin-mao
cc: @Matt-Esch @zhenghuiwang

coveralls · 2017-01-20T01:32:56Z

Coverage increased (+0.2%) to 91.253% when pulling 4bfab9f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-20T01:32:56Z

Coverage increased (+0.2%) to 91.253% when pulling 4bfab9f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-20T01:32:56Z

Coverage increased (+0.2%) to 91.253% when pulling 4bfab9f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-20T01:32:56Z

Coverage increased (+0.2%) to 91.253% when pulling 4bfab9f on cached-reporter into 4ecb8f9 on master.

robskillington · 2017-01-20T18:05:07Z

reporter.go

+	// CachedCapabilities returns a description of metrics reporting capabilities
+	CachedCapabilities() Capabilities
+
+	// CachedFlush is expected to be called by a Scope when it completes a round or reporting


Might be worth fixing the doc string here, the "or" is meant to be "of". Worth fixing on line 39 too I should think.

I think it might be worth renaming CachedCapabilities and CachedFlush to just Capabilities and Flush and then creating a new internal only interface:

type baseStatsReporter interface { Capabilities() Capabilities() Flush() }

That way you can add a field to the scope:

reporter StatsReporter // existing cachedReporter CachedStatsReporter // existing baseReporter baseStatsReporter // new

And when calling capabilities or flush:

// capabilities s.baseReporter.Capabilities() // flush s.baseReporter.Flush()

Just to avoid the if/elses there.

Fixed. However this means that the Flush() method name classes and a single struct can no longer implement both StatsReporter and CachedStatsReporter, but that's probably a good thing.

robskillington · 2017-01-20T18:40:59Z

scope.go

@@ -305,6 +376,9 @@ func (s *scope) Close() error {
 	if closer, ok := s.reporter.(io.Closer); ok {
 		return closer.Close()
 	}
+	if closer, ok := s.cachedReporter.(io.Closer); ok {
+		return closer.Close()
+	}


If we were using the baseStatsReporter approach this could also just be:

if closer, ok := s.baseReporter.(io.Closer); ok { return closer.Close() }

robskillington · 2017-01-20T18:43:27Z

stats.go

+	if prev == curr {
+		return
+	}
+	atomic.StoreInt64(&c.prev, curr)


Might be best to avoid repeating this code by adding a new method that takes the value?

func (c *counter) value() int64 { curr := atomic.LoadInt64(&c.curr) prev := atomic.LoadInt64(&c.prev) if prev == curr { return 0 } atomic.StoreInt64(&c.prev, curr) return curr - prev }

Then we don't have to repeat this code twice.

robskillington · 2017-01-20T18:44:32Z

stats.go

+func (g *gauge) cachedReport() {
+	if atomic.SwapUint64(&g.updated, 0) == 1 {
+		g.cachedGauge.ReportGauge(
+			math.Float64frombits(atomic.LoadUint64(&g.curr)),


Maybe same for the gauge, something that just performs the math.Float64frombits(atomic.LoadUint64(&g.curr)). The compiler will likely inline it so will stay fast but is more concise to read.

robskillington · 2017-01-20T18:50:07Z

stats.go

+		t.cachedTimer.ReportTimer(interval)
+	} else {
+		t.reporter.ReportTimer(t.name, t.tags, interval)
+	}


Might be worth binding this method to avoid the if/else jump at Record call time:

type timer struct { // ... reportFn func(interval time.Duration) } func newTimer(/*...*/) { t := &timer{/*...*/} if cachedTimer != nil { t.reportFn = t.cacheTimer.ReportTimer } else { t.reportFn = t.report } } func (t *timer) Record(interval time.Duration) { t.reportFn(interval) } func (t *timer) report(interval time.Duration) { t.reporter.ReportTimer(t.name, t.tags, interval) }

I tried this.

raynos at raynos-ThinkPad-T440p ~/gocode/src/github.com/uber-go/tally on cached-reporter* $ go test -bench BenchmarkTimerReport -run _NONE_ -cpuprofile cpuprofile.out -benchmem -benchtime 7s -cpu 2 . BenchmarkTimerReport-2 2000000000 6.02 ns/op 0 B/op 0 allocs/op PASS ok github.com/uber-go/tally 12.633s raynos at raynos-ThinkPad-T440p ~/gocode/src/github.com/uber-go/tally on cached-reporter* $ go test -bench BenchmarkTimerReport -run _NONE_ -cpuprofile cpuprofile.out -benchmem -benchtime 7s -cpu 2 . BenchmarkTimerReport-2 1000000000 9.24 ns/op 0 B/op 0 allocs/op PASS ok github.com/uber-go/tally 10.196s

It looks like the inline if statement is faster then the bound function call. Adding an extra function call takes 9/10ns instead of 5/6ns.

Interesting, sounds good. Thanks for the breakdown 👍

robskillington · 2017-01-20T18:51:57Z

scope.go

+	if s.cachedReporter != nil {
+		return s.cachedReporter.CachedCapabilities()
+	}
+	return capabilitiesNone


If we were using the baseStatsReporter approach this could also just be:

if s.baseReporter != nil [ return s.baseReporter.Capabilities() } return capabilitiesNone

coveralls · 2017-01-20T22:44:12Z

Coverage increased (+1.2%) to 92.308% when pulling 44cd59d on cached-reporter into 4ecb8f9 on master.

robskillington · 2017-01-21T00:26:37Z

reporter.go

@@ -22,8 +22,15 @@ package tally

 import "time"

+type baseStatsReporter interface {


Might be worth making this an exported interface on second thoughts since its strange to export an interface that extends a non-exported defined interface.

robskillington · 2017-01-21T00:28:21Z

Other than final comment LGTM

Raynos · 2017-01-21T01:27:40Z

@robskillington

After I land this we should cut a proper version and use glide versioning in tally.

Since I'm going to be using uber-go/tally in prod, I'm happy to cut 1.0.0 and lock down the interface ;)

Alternatively we can version it as 0.1.0 if there are known things that need to change.

coveralls · 2017-01-21T01:29:30Z

Coverage increased (+1.2%) to 92.308% when pulling e5e147f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-21T01:29:30Z

Coverage increased (+1.2%) to 92.308% when pulling e5e147f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-21T01:29:30Z

Coverage increased (+1.2%) to 92.308% when pulling e5e147f on cached-reporter into 4ecb8f9 on master.

coveralls · 2017-01-21T01:29:31Z

Coverage increased (+1.2%) to 92.308% when pulling e5e147f on cached-reporter into 4ecb8f9 on master.

robskillington

LGTM - @Raynos sounds good, I'm fine with cutting 1.0.0

We do need to add native histogram types soon but we can make that a 2.0.0 release.

Raynos · 2017-01-21T04:56:32Z

Ok, will wait until monday for landing, to catch more reviewers.

martin-mao

LGTM

Raynos · 2017-01-23T20:05:27Z

I've merged this and then started using semantic versioning in this repository, starting at v1.0.0

Raynos added 3 commits January 19, 2017 16:54

reporter: add interface for cached reporter

19ccf6b

scope: use CachedReporter and hook it up in stats

3d449ff

add more tests

4bfab9f

robskillington reviewed Jan 20, 2017

View reviewed changes

Raynos added 2 commits January 20, 2017 14:38

scope: add baseReporter interface

4f8fe7f

stats: remove atomic logic into shared methods

44cd59d

robskillington reviewed Jan 21, 2017

View reviewed changes

reporter: add BaseStatsReporter interface

e5e147f

robskillington approved these changes Jan 21, 2017

View reviewed changes

martin-mao approved these changes Jan 23, 2017

View reviewed changes

Raynos merged commit 9a10be9 into master Jan 23, 2017

Raynos deleted the cached-reporter branch January 23, 2017 19:56

anuptalwalkar mentioned this pull request Jan 27, 2017

NopCachedStatsReporter #23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CachedReporter interface that supports pre allocation. #22

Add CachedReporter interface that supports pre allocation. #22

Raynos commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

robskillington Jan 20, 2017

Raynos Jan 20, 2017

robskillington Jan 20, 2017 •

edited

Raynos Jan 20, 2017

robskillington Jan 20, 2017 •

edited

Raynos Jan 20, 2017

robskillington Jan 20, 2017

Raynos Jan 20, 2017

robskillington Jan 20, 2017

Raynos Jan 20, 2017

robskillington Jan 21, 2017

robskillington Jan 20, 2017

coveralls commented Jan 20, 2017 •

edited

robskillington Jan 21, 2017 •

edited

Raynos Jan 21, 2017

robskillington commented Jan 21, 2017

Raynos commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

robskillington left a comment

Raynos commented Jan 21, 2017

martin-mao left a comment

Raynos commented Jan 23, 2017

		@@ -22,8 +22,15 @@ package tally

		import "time"

		type baseStatsReporter interface {

Add CachedReporter interface that supports pre allocation. #22

Add CachedReporter interface that supports pre allocation. #22

Conversation

Raynos commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Jan 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Jan 20, 2017 • edited

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

coveralls commented Jan 20, 2017 • edited

robskillington Jan 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington commented Jan 21, 2017

Raynos commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

coveralls commented Jan 21, 2017

robskillington left a comment

Choose a reason for hiding this comment

Raynos commented Jan 21, 2017

martin-mao left a comment

Choose a reason for hiding this comment

Raynos commented Jan 23, 2017

robskillington Jan 20, 2017 •

edited

robskillington Jan 20, 2017 •

edited

coveralls commented Jan 20, 2017 •

edited

robskillington Jan 21, 2017 •

edited