Merge lp:~jameinel/juju-core/cache-facade-instances into lp:~go-bot/juju-core/trunk

[John A Meinel]

state/apiserver/root.go: cache active facades

Since I made Client no longer special, it was now being created again
for every request, which is a bit inefficient. However, since we now
have a centralized location where requests go through, it is pretty easy
to add caching there. And now all Facades are cached. It even comes with
a test that we don't create new objects for the same (Name, version)
couple, but we do create a different one when version changes.

https://codereview.appspot.com/96600043/

[John A Meinel (jameinel)]

Reviewers: mp+220937_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/root.go: cache active facades

Since I made Client no longer special, it was now being created again
for every request, which is a bit inefficient. However, since we now
have a centralized location where requests go through, it is pretty easy
to add caching there. And now all Facades are cached. It even comes with
a test that we don't create new objects for the same (Name, version)
couple, but we do create a different one when version changes.

https://code.launchpad.net/~jameinel/juju-core/cache-facade-instances/+merge/220937

Requires:
https://code.launchpad.net/~jameinel/juju-core/api-srvRoot-ensures-type/+merge/220615

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/96600043/

Affected files (+107, -28 lines):
   A [revision details]
   M state/apiserver/export_test.go
   M state/apiserver/root.go
   M state/apiserver/root_test.go

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/96600043/

[John A Meinel (jameinel)]

moving to WIP since this will all land together.

Unmerged revisions

2760. By John A Meinel on 2014-05-26

A good thing we added a test, because we weren't actually caching anything. :)

So now we have a proper test case that we can see the objects are
getting cached and reused.

2759. By John A Meinel on 2014-05-26

Merged api-srvRoot-ensures-type into cache-facade-instances.

2758. By John A Meinel on 2014-05-22

Merged api-srvRoot-ensures-type into cache-facade-instances.

2757. By John A Meinel on 2014-05-22

Merged api-srvRoot-ensures-type into cache-facade-instances.

2756. By John A Meinel on 2014-05-22

Merged api-srvRoot-ensures-type into cache-facade-instances.

2755. By John A Meinel on 2014-05-22

Merged api-srvRoot-ensures-type into cache-facade-instances.

2754. By John A Meinel on 2014-05-22

merge up the change to ensure the expectedType of Facade objects.

2753. By John A Meinel on 2014-05-20

Merge in api-registry-tracks-type, resolve conflict

2752. By John A Meinel on 2014-05-20

Use a generic object cache keyed on facade name and version.

2751. By John A Meinel on 2014-05-20

Merge api-use-register-standard-facade.

=== modified file 'state/apiserver/export_test.go'
--- state/apiserver/export_test.go	2014-05-26 10:15:11 +0000
+++ state/apiserver/export_test.go	2014-05-26 10:15:12 +0000
@@ -48,9 +48,10 @@
 // enough to actually do any RPC calls
 func TestingSrvRoot(st *state.State) *srvRoot {
 	return &srvRoot{
-		state:     st,
-		rpcConn:   nil,
-		resources: common.NewResources(),
-		entity:    nil,
+		state:       st,
+		rpcConn:     nil,
+		resources:   common.NewResources(),
+		entity:      nil,
+		objectCache: make(map[facadeKey]interface{}),
 	}
 }

=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2014-05-26 10:15:11 +0000
+++ state/apiserver/root.go	2014-05-26 10:15:12 +0000
@@ -33,13 +33,19 @@
 	mongoPingInterval = 10 * time.Second
 )
 
+type facadeKey struct {
+	name    string
+	version int
+}
+
 // srvRoot represents a single client's connection to the state
 // after it has logged in.
 type srvRoot struct {
-	state     *state.State
-	rpcConn   *rpc.Conn
-	resources *common.Resources
-	entity    taggedAuthenticator
+	state       *state.State
+	rpcConn     *rpc.Conn
+	resources   *common.Resources
+	entity      taggedAuthenticator
+	objectCache map[facadeKey]interface{}
 }
 
 // newSrvRoot creates the client's connection representation
@@ -47,10 +53,11 @@
 // connection.
 func newSrvRoot(root *initialRoot, entity taggedAuthenticator) *srvRoot {
 	r := &srvRoot{
-		state:     root.srv.state,
-		rpcConn:   root.rpcConn,
-		resources: common.NewResources(),
-		entity:    entity,
+		state:       root.srv.state,
+		rpcConn:     root.rpcConn,
+		resources:   common.NewResources(),
+		entity:      entity,
+		objectCache: make(map[facadeKey]interface{}),
 	}
 	// Note: Only the Client API is part of srvRoot rather than all
 	// the other ones being created on-demand. Why is that?
@@ -69,22 +76,31 @@
 // that has a Call() method that will invoke the concrete method on a live
 // object.
 func (r *srvRoot) MethodCaller(rootMethodName string, version int, objId, objMethodName string) (rpcreflect.MethodCaller, error) {
-	factory, err := common.GetFacadeFactory(rootMethodName, version)
-	if err != nil {
-		return rpcreflect.MethodCaller{}, err
-	}
-	expectedType, err := common.GetFacadeType(rootMethodName, version)
-	if err != nil {
-		return rpcreflect.MethodCaller{}, err
-	}
-	facade, err := factory(r.state, r.resources, r, objId)
-	if err != nil {
-		return rpcreflect.MethodCaller{}, err
-	}
-	if !reflect.TypeOf(facade).AssignableTo(expectedType) {
-		return rpcreflect.MethodCaller{}, fmt.Errorf(
-			"internal error, %s(%d) claimed to return %s but returned %T",
-			rootMethodName, version, expectedType, facade)
+	key := facadeKey{rootMethodName, version}
+	var facade interface{}
+	if obj, ok := r.objectCache[key]; ok {
+		logger.Tracef("cache hit for %s(%d)", rootMethodName, version)
+		facade = obj
+	} else {
+		logger.Tracef("cache miss for %s(%d)", rootMethodName, version)
+		factory, err := common.GetFacadeFactory(rootMethodName, version)
+		if err != nil {
+			return rpcreflect.MethodCaller{}, err
+		}
+		expectedType, err := common.GetFacadeType(rootMethodName, version)
+		if err != nil {
+			return rpcreflect.MethodCaller{}, err
+		}
+		facade, err = factory(r.state, r.resources, r, objId)
+		if err != nil {
+			return rpcreflect.MethodCaller{}, err
+		}
+		if !reflect.TypeOf(facade).AssignableTo(expectedType) {
+			return rpcreflect.MethodCaller{}, fmt.Errorf(
+				"internal error, %s(%d) claimed to return %s but returned %T",
+				rootMethodName, version, expectedType, facade)
+		}
+		r.objectCache[key] = facade
 	}
 	return rpcreflect.MethodCallerFromValue(
 		reflect.ValueOf(facade), rootMethodName, version, objMethodName)

=== modified file 'state/apiserver/root_test.go'
--- state/apiserver/root_test.go	2014-05-26 10:15:11 +0000
+++ state/apiserver/root_test.go	2014-05-26 10:15:12 +0000
@@ -105,3 +105,63 @@
 	c.Check(err, gc.ErrorMatches,
 		`internal error, my-testing-facade\(0\) claimed to return \*apiserver_test.testingType but returned \*apiserver_test.badType`)
 }
+
+type intVar struct {
+	I int64
+}
+
+type countingType struct {
+	i int64
+}
+
+func (ct *countingType) Count() intVar {
+	return intVar{ct.i}
+}
+
+func (ct *countingType) AltCount() intVar {
+	return intVar{ct.i}
+}
+
+func (r *rootSuite) TestMethodCallerCachesFacades(c *gc.C) {
+	srvRoot := apiserver.TestingSrvRoot(nil)
+	defer common.Facades.Discard("my-counting-facade", 0)
+	defer common.Facades.Discard("my-counting-facade", 1)
+	var count int64
+	newCounter := func(
+		*state.State, *common.Resources, common.Authorizer,
+	) (
+		*countingType, error,
+	) {
+		count += 1
+		return &countingType{count}, nil
+	}
+	common.RegisterStandardFacade("my-counting-facade", 0, newCounter)
+	common.RegisterStandardFacade("my-counting-facade", 1, newCounter)
+	// The first time we call MethodCaller, it should lookup a facade, and
+	// request a new object.
+	caller, err := srvRoot.MethodCaller("my-counting-facade", 0, "", "Count")
+	c.Assert(err, gc.IsNil)
+	v, err := caller.Call(reflect.Value{})
+	c.Assert(err, gc.IsNil)
+	c.Check(v.Interface(), gc.Equals, intVar{1})
+	// The second time we ask for a method on the same facade, it should
+	// reuse that object, rather than creating another instance
+	caller, err = srvRoot.MethodCaller("my-counting-facade", 0, "", "AltCount")
+	c.Assert(err, gc.IsNil)
+	v, err = caller.Call(reflect.Value{})
+	c.Assert(err, gc.IsNil)
+	c.Check(v.Interface(), gc.Equals, intVar{1})
+	// But when we ask for a different version, we should get a new
+	// instance
+	caller, err = srvRoot.MethodCaller("my-counting-facade", 1, "", "Count")
+	c.Assert(err, gc.IsNil)
+	v, err = caller.Call(reflect.Value{})
+	c.Assert(err, gc.IsNil)
+	c.Check(v.Interface(), gc.Equals, intVar{2})
+	// But it, too, should be cached
+	caller, err = srvRoot.MethodCaller("my-counting-facade", 1, "", "AltCount")
+	c.Assert(err, gc.IsNil)
+	v, err = caller.Call(reflect.Value{})
+	c.Assert(err, gc.IsNil)
+	c.Check(v.Interface(), gc.Equals, intVar{2})
+}