Add a simple cache for objects stored in etcd #7288

fgrzadkowski · 2015-04-24T12:36:22Z

This is a prototype of a simple cache that stores objects stored in etcd. It uses ModifiedIndex as a key, assuming that each modification changes only one object.

It's pretty hard to benchmark effect of this change, but I have measurements and observations:

e2e performance tests (100 nodes, 3000 pods) runs ~25% faster (from ~20 min down to ~15 min)
CPU profiles shows that conversion code is now below 20% (usually around 10%) where it used to be up to 70% of CPU time
cache hit ratio is 100:1 (100 reads for 1 write) or more

@wojtek-t @davidopp @lavalamp @timothysc @smarterclayton

smarterclayton · 2015-04-24T12:42:38Z

pkg/tools/etcd_helper.go

+func (h *EtcdHelper) GetFromCache(index uint64) (obj reflect.Value, ok bool) {
+	h.mutex.RLock()
+	defer h.mutex.RUnlock()
+	obj, ok = h.cache[index]


Would need to do a copy here to prevent people from mutating the cache - add that so your benchmarks are accurate.

Yeah, this is unfortunately true.

But you can do the copy after unlocking.

lavalamp · 2015-04-24T18:03:54Z

pkg/tools/etcd_helper.go

@@ -38,6 +39,8 @@ type EtcdHelper struct {
 	Codec  runtime.Codec
 	// optional, no atomic operations can be performed without this interface
 	Versioner EtcdVersioner
+	cache     map[uint64]reflect.Value


Please add a comment explaining this cache... including why you chose reflect.Value to store instead of runtime.Object, that it depends on etcd's indexes being globally unique per object (so if we make multiple etcd clusters, we'd need to include object key as part of this key), and that it's limited to N entries.

lavalamp · 2015-04-24T18:09:37Z

I like this a lot more than I was expecting to. :) But I fear adding a deep copy call in the cache is going to give back most of the performance gains. :(

smarterclayton · 2015-04-24T18:22:08Z

My money is on it'll be 10% better.

I'd really like to wait on the local caching until we have the unified watcher from etcd. Then we will be in a much better place to impose "fake etcdhelper". Just my 2c

----- Original Message -----

I like this a lot more than I was expecting to. :) But I fear adding a deep
copy call in the cache is going to give back most of the performance gains.
:(

Reply to this email directly or view it on GitHub:
#7288 (comment)

smarterclayton · 2015-04-27T12:43:34Z

pkg/tools/etcd_helper.go

+const maxEtcdCacheEntries int = 100000
+
+func (h *EtcdHelper) getFromCache(index uint64) (interface{}, bool) {
+	var obj interface{}


What's the measured performance of this after you make these changes?

Unfortunately we don't have a good metric to measure this, so I will use the same as I've used initially:

e2e performance tests still run ~25% faster (from ~20 min down to ~15 min)

cpu usage is slightly higher and profiling shows that DeepCopy uses up to 30% (usually around 10%). Still CPU is not saturated and uses 100% only rarely (but is very close).

Also latency metrics didn't degrade - 99%tile of api call, except list pods, run below 1s (checked manually based on apiserver metrics).

On a related note - we need a better metric, than e2e total time.

----- Original Message -----

} return nil

}

+// etcdCache defines interface used for caching objects stored in etcd.
Objects are keyed by
+// their Node.ModifiedIndex, which is unique across all types.
+// All implementations must be thread-safe.
+type etcdCache interface {

getFromCache(index uint64) (interface{}, bool)

addToCache(index uint64, obj interface{})
+}

+const maxEtcdCacheEntries int = 100000
+
+func (h *EtcdHelper) getFromCache(index uint64) (interface{}, bool) {

var obj interface{}

Unfortunately we don't have a good metric to measure this, so I will the same
as I've used initially:

e2e performance tests still run ~25% faster (from ~20 min down to ~15 min)

cpu usage is slightly higher and profiling shows that DeepCopy uses up to
30% (usually around 10%). Still CPU is not saturated and uses 100% only
rarely (but is very close).

How much more memory?

Fortunately, the work being done for conversion can be used to implement efficient DeepCopy, so I would expect that portion to go down.

Also latency metrics didn't degrade - 99%tile of api call, except list pods,
run below 1s (checked manually based on apiserver metrics).

On a related note - we need a better metric, than e2e total time.

Agree.

Fortunately, the work being done for conversion can be used to implement efficient DeepCopy, so I would expect that portion to go down.

Yeah, but that work should also make this caching mechanism mostly unnecessary, after that is finished this will only save on unmarshalling costs.

@smarterclayton I checked memory and (surprisingly) memory footprint is much lower with cache. Maximum reserved memory during performance tests (100 nodes, 3000 pods) is:

2.9 GB without cache

1.8 GB with cache
It seems that parsing JSON uses a lot of memory. If it's done in multliple goroutines concurrently it can affect memory usage pretty significantly

@lavalamp I think that even with fast conversions we will still benefit from cache due to:

reduce memory footprint

unmarshalling JSON can actually use a lot of CPU (I've seen profiles with >20%)

On Apr 28, 2015, at 7:21 AM, Filip Grzadkowski notifications@github.com wrote:

In pkg/tools/etcd_helper.go:

} return nil

}

+// etcdCache defines interface used for caching objects stored in etcd. Objects are keyed by
+// their Node.ModifiedIndex, which is unique across all types.
+// All implementations must be thread-safe.
+type etcdCache interface {

getFromCache(index uint64) (interface{}, bool)

addToCache(index uint64, obj interface{})
+}

+const maxEtcdCacheEntries int = 100000
+
+func (h *EtcdHelper) getFromCache(index uint64) (interface{}, bool) {

var obj interface{}
@smarterclayton I checked memory and (surprisingly) memory footprint is much lower with cache. Maximum reserved memory during performance tests (100 nodes, 3000 pods) is:

2.9 GB without cache
1.8 GB with cache It seems that parsing JSON uses a lot of memory. If it's done in multliple goroutines concurrently it can affect memory usage pretty significantly
@lavalamp I think that even with fast conversions we will still benefit from cache due to:

reduce memory footprint
unmarshalling JSON can actually use a lot of CPU (I've seen profiles with >20%)
I'd like I see a quick investigation of ugorji after the conversion work is in place - if unmarshalling JSON is making a significant memory impact and ugorji is cheap to setup (since I believe it can fall back to the default json serializer implementation), it may be a very quick win for us.
—
Reply to this email directly or view it on GitHub.

fgrzadkowski · 2015-04-27T15:42:04Z

I addressed all of the initial comments, tests pass so this PR is ready for normal review.

lavalamp · 2015-04-27T18:07:47Z

pkg/tools/etcd_helper_watch_test.go

@@ -205,7 +217,7 @@ func TestWatchEtcdError(t *testing.T) {
 	fakeClient := NewFakeEtcdClient(t)
 	fakeClient.expectNotFoundGetSet["/some/key"] = struct{}{}
 	fakeClient.WatchImmediateError = fmt.Errorf("immediate error")
-	h := EtcdHelper{fakeClient, codec, versioner}
+	h := NewEtcdHelper(fakeClient, codec)


I'm a little worried about removing the versioner setting here-- it means that this test will test inconsistent sets of things if someone changes the "versioner" at the top of the file OR if someone changes the versioner that NewEtcdHelper uses. It's not too likely to change so I'm not too worried, just vaguely uneasy.

That's true, but on the other hand we want test setup that is consistent with prod.

lavalamp · 2015-04-27T18:11:51Z

LGTM-- travis appears to have found a data race that you should probably figure out before we merge.

lavalamp · 2015-04-27T18:13:59Z

pkg/tools/etcd_helper.go

+	// have to revisited if we decide to do things like multiple etcd clusters, or etcd will
+	// support multi-object transaction that will result in many objects with the same index.
+	// Number of entries stored in the cache is controlled by maxEtcdCacheEntries constant.
+	cache map[uint64]interface{}


nit: recommend storing runtime.Object instead of interface{}.

But that would require additional casting, which doesn't improve readability.

It may not improve readability, but it tells readers a very important feature of the objects they will find in this cache. Types should be as narrowly scoped as possible.

timothysc · 2015-04-28T14:24:44Z

Won't there be a consistency issue here if we have multiple apiservers?

@rrati ^ FYI.

fgrzadkowski · 2015-04-28T14:29:34Z

@timothysc If we have multiple apiservers, we do not need the same cache entries. We only require that etcd returns the same ModifiedIndex for the same objects (which will be the case).

lavalamp · 2015-04-28T18:14:50Z

Can you get numbers again with the second deep copy?

Also I really would perfer to use runtime.Object as the type of the cache...

fgrzadkowski · 2015-04-29T12:38:49Z

DeepCopy in write path does not change anything, because of hit ration (1 write for ~100 reads)

Per request changed to runtime.Object.

fgrzadkowski · 2015-04-29T12:40:03Z

e2e tests pass.

smarterclayton · 2015-04-29T14:20:53Z

LGTM, no more comments.

----- Original Message -----

e2e tests pass.

Reply to this email directly or view it on GitHub:
#7288 (comment)

lavalamp · 2015-04-29T16:00:28Z

LGTM

Add a simple cache for objects stored in etcd

timothysc · 2015-04-30T13:26:26Z

What's the status now? Looks like it got reverted.
@fgrzadkowski, @cjcullen

wojtek-t · 2015-04-30T13:29:10Z

@timothysc - yes it was reverted, but the hopfully fixed version is send out for review - see #7559

googlebot added the cla: yes label Apr 24, 2015

smarterclayton reviewed Apr 24, 2015
View reviewed changes

fgrzadkowski mentioned this pull request Apr 24, 2015

Too much time spent in net.(*netFD).dial #7160

Closed

lavalamp self-assigned this Apr 24, 2015

lavalamp reviewed Apr 24, 2015
View reviewed changes

fgrzadkowski mentioned this pull request Apr 27, 2015

Get pods for many pods is very heavy #6514

Closed

fgrzadkowski force-pushed the perf branch 2 times, most recently from 179bba0 to 0bef09e Compare April 27, 2015 11:33

smarterclayton reviewed Apr 27, 2015
View reviewed changes

fgrzadkowski force-pushed the perf branch from 0bef09e to 8eb4339 Compare April 27, 2015 15:34

fgrzadkowski changed the title ~~[WIP] Add a simple cache for objects stored in etcd.~~ Add a simple cache for objects stored in etcd Apr 27, 2015

lavalamp reviewed Apr 27, 2015
View reviewed changes

fgrzadkowski force-pushed the perf branch 4 times, most recently from 1434872 to 965c0ba Compare April 28, 2015 11:14

Add a simple cache for objects stored in etcd.

016e201

fgrzadkowski force-pushed the perf branch from 965c0ba to 016e201 Compare April 29, 2015 11:14

lavalamp added a commit that referenced this pull request Apr 29, 2015

Merge pull request #7288 from fgrzadkowski/perf

2802b18

Add a simple cache for objects stored in etcd

lavalamp merged commit 2802b18 into kubernetes:master Apr 29, 2015

cjcullen mentioned this pull request Apr 29, 2015

Revert "Add a simple cache for objects stored in etcd" #7516

Merged

fgrzadkowski mentioned this pull request Apr 30, 2015

Add a simple cache for objects stored in etcd. #7559

Merged

fgrzadkowski unassigned lavalamp Aug 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple cache for objects stored in etcd #7288

Add a simple cache for objects stored in etcd #7288

fgrzadkowski commented Apr 24, 2015

smarterclayton Apr 24, 2015

lavalamp Apr 24, 2015

lavalamp Apr 24, 2015

lavalamp Apr 24, 2015

fgrzadkowski Apr 27, 2015

lavalamp commented Apr 24, 2015

smarterclayton commented Apr 24, 2015

smarterclayton Apr 27, 2015

fgrzadkowski Apr 27, 2015

smarterclayton Apr 27, 2015

lavalamp Apr 27, 2015

fgrzadkowski Apr 28, 2015

smarterclayton Apr 28, 2015

fgrzadkowski commented Apr 27, 2015

lavalamp Apr 27, 2015

fgrzadkowski Apr 28, 2015

lavalamp commented Apr 27, 2015

lavalamp Apr 27, 2015

fgrzadkowski Apr 28, 2015

lavalamp Apr 28, 2015

timothysc commented Apr 28, 2015

fgrzadkowski commented Apr 28, 2015

lavalamp commented Apr 28, 2015

fgrzadkowski commented Apr 29, 2015

fgrzadkowski commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

lavalamp commented Apr 29, 2015

timothysc commented Apr 30, 2015

wojtek-t commented Apr 30, 2015

Add a simple cache for objects stored in etcd #7288

Add a simple cache for objects stored in etcd #7288

Conversation

fgrzadkowski commented Apr 24, 2015

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

lavalamp commented Apr 24, 2015

smarterclayton commented Apr 24, 2015

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

fgrzadkowski commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Apr 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysc commented Apr 28, 2015

fgrzadkowski commented Apr 28, 2015

lavalamp commented Apr 28, 2015

fgrzadkowski commented Apr 29, 2015

fgrzadkowski commented Apr 29, 2015

smarterclayton commented Apr 29, 2015

lavalamp commented Apr 29, 2015

timothysc commented Apr 30, 2015

wojtek-t commented Apr 30, 2015