store: cache the statistics token ids

store: cache the statistics token ids

This adds caching with two generations of 512 entries each.

https://code.launchpad.net/~niemeyer/juju/go-store-stats-caching/+merge/102227

Requires: https://code.launchpad.net/~niemeyer/juju/go-store-stats-readonly/+merge/101673

(do not edit description out of merge proposal)

[niemeyer, 2012-04-17 02:18:08]

Please take a look.

[rog, 2012-04-17 09:11:14]

nice stuff.

https://codereview.appspot.com/6055044/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode44
store/store.go:44: // Cache for statistics key words (two generations).
i like this scheme.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode46
store/store.go:46: statsToken1 map[string]int
i had to look quite closely to work out which one of these is fresher than the
other. perhaps cacheTokens and cacheTokensOld ?

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode121
store/store.go:121: var err error
+1

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode125
store/store.go:125: var t struct{ Id int "_id"; Token string "t" }
gofmt

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode162
store/store.go:162: if len(s.statsToken1) == statsTokenCacheSize {
s/==/>=/
defensively.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode167
store/store.go:167: s.statsToken1 = make(map[string]int, statsTokenCacheSize)
i'm slightly concerned that as soon as the size of the working set rises above
statsTokenCacheSize, we're going to be doing this all the time, but i can't
think of a good heuristic for adjusting the size.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode174
store/store.go:174: func (s *Store) cachedStatsTokenId(token string) (id int,
found bool) {
the visual distinction between these two function names is very small. how about
cacheStatsToken and statsToken?

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode178
store/store.go:178: if s.statsToken1 == nil {
this is unnecessary.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode185
store/store.go:185: if s.statsToken2 == nil {
this is unnecessary.

https://codereview.appspot.com/6055044/diff/1/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store_test.go#newcode525
store/store_test.go:525: func (s *StoreSuite) TestCountersTokenCaching(c *C) {
nice test.

https://codereview.appspot.com/6055044/diff/1/store/store_test.go#newcode531
store/store_test.go:531: // of 512 entries each.
i don't like the fact that 512 is hard coded here. we'll likely want to adjust
the cache size to tweak server performance. perhaps consider having an
export_test.go that exports the max cache size.

[fwereade, 2012-04-17 13:42:29]

LGTM; might be nice to make the generation size configurable (without rebuilding
;)) but I think that can wait until there's a clear need.

https://codereview.appspot.com/6055044/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode162
store/store.go:162: if len(s.statsToken1) == statsTokenCacheSize {
On 2012/04/17 09:11:14, rog wrote:
> s/==/>=/
> defensively.
+1

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode170
store/store.go:170: s.cacheMu.Unlock()
Why not defer the unlock? (I know there aren't any early returns *now*, but
isn't it just one more thing for a future maintainer to maybe forget?)

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode174
store/store.go:174: func (s *Store) cachedStatsTokenId(token string) (id int,
found bool) {
On 2012/04/17 09:11:14, rog wrote:
> the visual distinction between these two function names is very small. how
about
> cacheStatsToken and statsToken?

+1

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode178
store/store.go:178: if s.statsToken1 == nil {
On 2012/04/17 09:11:14, rog wrote:
> this is unnecessary.
I don't see any guarantee that this is filled in anywhere. cached... is called
before cache..., isn't it?

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode185
store/store.go:185: if s.statsToken2 == nil {
On 2012/04/17 09:11:14, rog wrote:
> this is unnecessary.
likewise; there's no guarantee that we've started to populate the 2nd gen cache,
is there?

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode189
store/store.go:189: s.cacheMu.RUnlock()
This does make perfect sense, but it stopped me short on first read... and I
can't actually think of anything clearer :(.

https://codereview.appspot.com/6055044/diff/1/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store_test.go#newcode531
store/store_test.go:531: // of 512 entries each.
On 2012/04/17 09:11:14, rog wrote:
> i don't like the fact that 512 is hard coded here. we'll likely want to adjust
> the cache size to tweak server performance. perhaps consider having an
> export_test.go that exports the max cache size.

+1

[niemeyer, 2012-04-17 18:43:37]

*** Submitted:

store: cache the statistics token ids

This adds caching with two generations of 512 entries each.

R=rog, fwereade
CC=
https://codereview.appspot.com/6055044

https://codereview.appspot.com/6055044/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode44
store/store.go:44: // Cache for statistics key words (two generations).
On 2012/04/17 09:11:14, rog wrote:
> i like this scheme.

Cheers.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode46
store/store.go:46: statsToken1 map[string]int
On 2012/04/17 09:11:14, rog wrote:
> i had to look quite closely to work out which one of these is fresher than the
> other. perhaps cacheTokens and cacheTokensOld ?

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode125
store/store.go:125: var t struct{ Id int "_id"; Token string "t" }
On 2012/04/17 09:11:14, rog wrote:
> gofmt

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode162
store/store.go:162: if len(s.statsToken1) == statsTokenCacheSize {
On 2012/04/17 13:42:29, fwereade wrote:
> On 2012/04/17 09:11:14, rog wrote:
> > s/==/>=/
> > defensively.
> +1

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode167
store/store.go:167: s.statsToken1 = make(map[string]int, statsTokenCacheSize)
On 2012/04/17 09:11:14, rog wrote:
> i'm slightly concerned that as soon as the size of the working set rises above
> statsTokenCacheSize, we're going to be doing this all the time, but i can't
> think of a good heuristic for adjusting the size.

We can tweak the cache size, which is an issue with any cache, but I really
don't see a real problem here. It's a single value allocated every cache-size
iterations in the worst case scenario.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode170
store/store.go:170: s.cacheMu.Unlock()
On 2012/04/17 13:42:29, fwereade wrote:
> Why not defer the unlock? (I know there aren't any early returns *now*, but
> isn't it just one more thing for a future maintainer to maybe forget?)

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode174
store/store.go:174: func (s *Store) cachedStatsTokenId(token string) (id int,
found bool) {
On 2012/04/17 13:42:29, fwereade wrote:
> On 2012/04/17 09:11:14, rog wrote:
> > the visual distinction between these two function names is very small. how
> about
> > cacheStatsToken and statsToken?
> 
> +1

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode178
store/store.go:178: if s.statsToken1 == nil {
On 2012/04/17 09:11:14, rog wrote:
> this is unnecessary.

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode178
store/store.go:178: if s.statsToken1 == nil {
On 2012/04/17 13:42:29, fwereade wrote:
> On 2012/04/17 09:11:14, rog wrote:
> > this is unnecessary.
> I don't see any guarantee that this is filled in anywhere. cached... is called
> before cache..., isn't it?

What Rog means is that it's fine to access nil maps. It originally wasn't fine,
but now it is.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode185
store/store.go:185: if s.statsToken2 == nil {
On 2012/04/17 09:11:14, rog wrote:
> this is unnecessary.

Done.

https://codereview.appspot.com/6055044/diff/1/store/store.go#newcode189
store/store.go:189: s.cacheMu.RUnlock()
On 2012/04/17 13:42:29, fwereade wrote:
> This does make perfect sense, but it stopped me short on first read... and I
> can't actually think of anything clearer :(.

Given the simplifications above, I've changed to not use defer, which made the
logic a bit nicer.

https://codereview.appspot.com/6055044/diff/1/store/store_test.go
File store/store_test.go (right):

https://codereview.appspot.com/6055044/diff/1/store/store_test.go#newcode531
store/store_test.go:531: // of 512 entries each.
On 2012/04/17 09:11:14, rog wrote:
> i don't like the fact that 512 is hard coded here. we'll likely want to adjust
> the cache size to tweak server performance. perhaps consider having an
> export_test.go that exports the max cache size.

Introducing a new file to export a single constant in this specific situation
seems unnecessary considering that this test will necessarily break if the
internal variable is changed. I agree having constants spread here is a poor
choice, though. I've introduced a genSize constant that is used across the test.
If we want to change the size, we have to tweak two locations only, and they're
both tied by the test.