Make the rest of the services have a similar raw/user interface structure.

impl/memory/memcache.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package memory
 
 import (
+        "encoding/binary"
         "sync"
         "time"
 
         "golang.org/x/net/context"
 
-        "github.com/luci/gae/impl/dummy"
         mc "github.com/luci/gae/service/memcache"
         "github.com/luci/luci-go/common/clock"
+        "github.com/luci/luci-go/common/errors"
 )
 
 type mcItem struct {
         key        string
         value      []byte
-        object     interface{}
         flags      uint32
         expiration time.Duration
 
         CasID uint64
 }
 
 var _ mc.Item = (*mcItem)(nil)
 
 func (m *mcItem) Key() string               { return m.key }
 func (m *mcItem) Value() []byte             { return m.value }
-func (m *mcItem) Object() interface{}       { return m.object }
 func (m *mcItem) Flags() uint32             { return m.flags }
 func (m *mcItem) Expiration() time.Duration { return m.expiration }
 
 func (m *mcItem) SetKey(key string) mc.Item {
         m.key = key
         return m
 }
 func (m *mcItem) SetValue(val []byte) mc.Item {
         m.value = val
         return m
 }
-func (m *mcItem) SetObject(obj interface{}) mc.Item {
-        m.object = obj
-        return m
-}
 func (m *mcItem) SetFlags(flg uint32) mc.Item {
         m.flags = flg
         return m
 }
 func (m *mcItem) SetExpiration(exp time.Duration) mc.Item {
         m.expiration = exp
         return m
 }
 
-func (m *mcItem) duplicate() *mcItem {
+func (m *mcItem) SetAll(other mc.Item) {
+        *m = *other.(*mcItem).duplicate(false)
+}
+
+func (m *mcItem) duplicate(deep bool) *mcItem {
         ret := mcItem{}
         ret = *m
-        ret.value = make([]byte, len(m.value))
-        copy(ret.value, m.value)
+        if deep {
+                ret.value = make([]byte, len(m.value))
+                copy(ret.value, m.value)
+        }
         return &ret
 }
 
 type memcacheData struct {
-        lock  sync.Mutex
+        lock  sync.RWMutex
         items map[string]*mcItem
         casID uint64
+
+        stats mc.Statistics
+}
+
+func (m *memcacheData) mkItemLocked(now time.Time, i mc.Item) (ret *mcItem) {
+        m.casID++
+
+        var exp time.Duration

[dnj, 2015/08/03 22:37:25]

Maybe:
exp := i.Expiration()
if exp != 0 {
  exp = now.Add(exp)
}

Why are you storing the expiration time as a "time.Duration"? It seems like it
should be a "time.Time". The math here is basically encapsulating a time.Time in
a time.Duration, which seems weird to me.

Also, why are you setting the expiration if it's already set? Is this a reset?
I'd imagine that you should set the expiration if it == 0, not != 0?

[iannucci, 2015/08/04 01:21:20]

https://github.com/golang/appengine/blob/master/memcache/memcache.go#L80
? I'm calculating the expiration based on the input expiration and the current
time, unless the expiration is 0, which is special.

I could change the internal representation I guess... This isn't new code, this
is just moved from before.

+        if i.Expiration() != 0 {
+                exp = time.Duration(now.Add(i.Expiration()).UnixNano())
+        }
+        newItem := mcItem{
+                key:        i.Key(),
+                flags:      i.Flags(),
+                expiration: exp,
+                value:      i.Value(),
+                CasID:      m.casID,
+        }
+        return newItem.duplicate(true)

[dnj, 2015/08/03 22:37:25]

This seems like a weird way of doing this. Allocate an object, populate it,
duplicate it, discard it, and return the duplicate. Why not just:

val := i.Value()
valCopy := make([]byte, len(val))
copy(valCopy, val)

return &mcItem{
  ...
  value: valCopy,
  ...
}

[iannucci, 2015/08/04 01:21:20]

Done

+}
+
+func (m *memcacheData) setItemLocked(now time.Time, i mc.Item) {
+        if cur, ok := m.items[i.Key()]; ok {
+                m.stats.Items--
+                m.stats.Bytes -= uint64(len(cur.value))
+        }
+        m.stats.Items++
+        m.stats.Bytes += uint64(len(i.Value()))
+        m.items[i.Key()] = m.mkItemLocked(now, i)

[dnj, 2015/08/03 22:37:25]

Let "m.items" be allowed to be nil and test/init here.

[iannucci, 2015/08/04 01:21:20]

Why?

+}
+
+func (m *memcacheData) delItemLocked(k string) {
+        if itm, ok := m.items[k]; ok {
+                m.stats.Items--
+                m.stats.Bytes -= uint64(len(itm.value))
+                delete(m.items, k)
+        }
+}
+
+func (m *memcacheData) reset() {
+        m.stats = mc.Statistics{}
+        m.items = map[string]*mcItem{}

[dnj, 2015/08/03 22:37:25]

Should just "nil" it here.

[iannucci, 2015/08/04 01:21:20]

Why?

[dnj (Google), 2015/08/04 03:48:03]

No reason to keep allocated empty maps around.

[iannucci, 2015/08/04 03:58:33]

yeah, but if it doesn't have data in it, then you're doing memcache wrong, and
allowing it to be nil moves initialization to less obvious places. 

[dnj, 2015/08/04 18:01:16]

Makes sense :)

+}
+
+func (m *memcacheData) hasLocked(now time.Time, key string) bool {

[dnj, 2015/08/03 22:37:25]

I read this as "has locked?". Maybe rename, "hasItemLocked" for consistency?

[iannucci, 2015/08/04 01:21:20]

Done.

+        ret, ok := m.items[key]
+        if ok && ret.Expiration() != 0 && ret.Expiration() < time.Duration(now.UnixNano()) {

[dnj, 2015/08/03 22:37:24]

Yeah ret.Expiration() really should just be a time.Time. Use
"time.Time{}.IsZero()" to test for zero value.

[iannucci, 2015/08/04 01:21:20]

I could do that in another CL. It'll cause more incompatibility with the
production implementation though, so I'm not sure why I would want to do this?

[dnj (Google), 2015/08/04 03:48:03]

Storing nanosecond since epoch in a time.Duration is not an appropriate use of
the type. time.Duration is an unanchored time delta. While one can argue it's an
implicit delta from epoch, but since a specific point in time plus a
time.Duration is a time.Time, we might as well use that type :)

[iannucci, 2015/08/04 03:58:33]

Yes, that's fine, but the sdk memcache API uses time.Duration. I don't want to
change the internal datatype in this CL, since it's big enough already, and I
don't want to explore all the places I'll have to patch up by changing this. So,
separate CL.

[dnj, 2015/08/04 18:01:16]

Acknowledged.

+                m.delItemLocked(key)
+                return false
+        }
+        return ok
+}
+
+func (m *memcacheData) retrieveLocked(now time.Time, key string) (*mcItem, error) {
+        if !m.hasLocked(now, key) {
+                m.stats.Misses++
+                return nil, mc.ErrCacheMiss
+        }
+
+        ret := m.items[key]
+        m.stats.Hits++
+        m.stats.ByteHits += uint64(len(ret.value))
+        return ret, nil
 }
 
 // memcacheImpl binds the current connection's memcache data to an
 // implementation of {gae.Memcache, gae.Testable}.
 type memcacheImpl struct {
-        mc.Interface
-
         data *memcacheData
         ctx  context.Context
 }
 
-var _ mc.Interface = (*memcacheImpl)(nil)
+var _ mc.RawInterface = (*memcacheImpl)(nil)
 
 // useMC adds a gae.Memcache implementation to context, accessible
 // by gae.GetMC(c)
 func useMC(c context.Context) context.Context {
         lck := sync.Mutex{}
         mcdMap := map[string]*memcacheData{}
 
-        return mc.SetFactory(c, func(ic context.Context) mc.Interface {
+        return mc.SetRawFactory(c, func(ic context.Context) mc.RawInterface {
                 lck.Lock()
                 defer lck.Unlock()
 
                 ns := curGID(ic).namespace
                 mcd, ok := mcdMap[ns]
                 if !ok {
                         mcd = &memcacheData{items: map[string]*mcItem{}}
                         mcdMap[ns] = mcd
                 }
 
                 return &memcacheImpl{
-                        dummy.Memcache(),
                         mcd,
                         ic,
                 }
         })
 }
 
-func (m *memcacheImpl) mkItemLocked(i mc.Item) (ret *mcItem) {
-        m.data.casID++
-
-        var exp time.Duration
-        if i.Expiration() != 0 {
-                exp = time.Duration(clock.Now(m.ctx).Add(i.Expiration()).UnixNano())
-        }
-        newItem := mcItem{
-                key:        i.Key(),
-                flags:      i.Flags(),
-                expiration: exp,
-                value:      i.Value(),
-                CasID:      m.data.casID,
-        }
-        return newItem.duplicate()
-}
-
 func (m *memcacheImpl) NewItem(key string) mc.Item {
         return &mcItem{key: key}
 }
 
-// Add implements context.MCSingleReadWriter.Add.
-func (m *memcacheImpl) Add(i mc.Item) error {
+func (m *memcacheImpl) AddMulti(items []mc.Item, cb mc.RawCB) error {
         m.data.lock.Lock()
         defer m.data.lock.Unlock()
 
-        if _, ok := m.retrieveLocked(i.Key()); !ok {
-                m.data.items[i.Key()] = m.mkItemLocked(i)
-                return nil
-        }
-        return mc.ErrNotStored
-}
-
-// CompareAndSwap implements context.MCSingleReadWriter.CompareAndSwap.
-func (m *memcacheImpl) CompareAndSwap(item mc.Item) error {
-        m.data.lock.Lock()
-        defer m.data.lock.Unlock()
-
-        if cur, ok := m.retrieveLocked(item.Key()); ok {
-                casid := uint64(0)
-                if mi, ok := item.(*mcItem); ok && mi != nil {
-                        casid = mi.CasID
+        for _, itm := range items {
+                now := clock.Now(m.ctx)

[dnj, 2015/08/03 22:37:25]

Do this once outside of the lock.

[iannucci, 2015/08/04 01:21:20]

Done.

+                if !m.data.hasLocked(now, itm.Key()) {
+                        m.data.setItemLocked(now, itm)
+                        cb(nil)
+                } else {
+                        cb(mc.ErrNotStored)
                 }
-
-                if cur.CasID == casid {
-                        m.data.items[item.Key()] = m.mkItemLocked(item)
-                } else {
-                        return mc.ErrCASConflict
-                }
-        } else {
-                return mc.ErrNotStored
         }
         return nil
 }
 
-// Set implements context.MCSingleReadWriter.Set.
-func (m *memcacheImpl) Set(i mc.Item) error {
+func (m *memcacheImpl) CompareAndSwapMulti(items []mc.Item, cb mc.RawCB) error {
         m.data.lock.Lock()
         defer m.data.lock.Unlock()
-        m.data.items[i.Key()] = m.mkItemLocked(i)
+
+        for _, itm := range items {
+                now := clock.Now(m.ctx)

[dnj, 2015/08/03 22:37:25]

Do this once outside of the lock.

[iannucci, 2015/08/04 01:21:20]

Done.

+                err := error(nil)
+                cur := (*mcItem)(nil)
+                if cur, err = m.data.retrieveLocked(now, itm.Key()); err == nil {
+                        casid := uint64(0)
+                        if mi, ok := itm.(*mcItem); ok && mi != nil {
+                                casid = mi.CasID
+                        }
+
+                        if cur.CasID == casid {
+                                m.data.setItemLocked(now, itm)
+                        } else {
+                                err = mc.ErrCASConflict
+                        }
+                } else {
+                        err = mc.ErrNotStored
+                }
+                cb(err)
+        }
+
         return nil
 }
 
-// Get implements context.MCSingleReadWriter.Get.
-func (m *memcacheImpl) Get(key string) (itm mc.Item, err error) {
+func (m *memcacheImpl) SetMulti(items []mc.Item, cb mc.RawCB) error {
         m.data.lock.Lock()
         defer m.data.lock.Unlock()
-        if val, ok := m.retrieveLocked(key); ok {
-                itm = val.duplicate().SetExpiration(0)
-        } else {
-                err = mc.ErrCacheMiss
+        for _, itm := range items {
+                m.data.setItemLocked(clock.Now(m.ctx), itm)

[dnj, 2015/08/03 22:37:25]

Get "now" once, outside of the lock/loop.

[iannucci, 2015/08/04 01:21:20]

Done.

+                cb(nil)

[dnj, 2015/08/03 22:37:25]

Consider setting the items within the lock, but doing the callbacks outside of
the lock.

[iannucci, 2015/08/04 01:21:20]

Done.

         }
-        return
+        return nil
 }
 
-// Delete implements context.MCSingleReadWriter.Delete.
-func (m *memcacheImpl) Delete(key string) error {
+func (m *memcacheImpl) GetMulti(keys []string, cb mc.RawItemCB) error {
+        m.data.lock.RLock()
+        defer m.data.lock.RUnlock()
+
+        for _, k := range keys {
+                itm := (mc.Item)(nil)
+                val, err := m.data.retrieveLocked(clock.Now(m.ctx), k)

[dnj, 2015/08/03 22:37:25]

Get "now" once, outside of the lock/loop.

[iannucci, 2015/08/04 01:21:20]

Done.

+                if err == nil {
+                        itm = val.duplicate(true).SetExpiration(0)
+                }
+                cb(itm, err)
+        }
+
+        return nil
+}
+
+func (m *memcacheImpl) DeleteMulti(keys []string, cb mc.RawCB) error {
         m.data.lock.Lock()
         defer m.data.lock.Unlock()
 
-        if _, ok := m.retrieveLocked(key); ok {
-                delete(m.data.items, key)
-                return nil
+        for _, k := range keys {
+                _, err := m.data.retrieveLocked(clock.Now(m.ctx), k)
+                if err == nil {
+                        m.data.delItemLocked(k)
+                }
+                cb(err)
         }
-        return mc.ErrCacheMiss
+
+        return nil
 }
 
-func (m *memcacheImpl) retrieveLocked(key string) (*mcItem, bool) {
-        ret, ok := m.data.items[key]
-        if ok && ret.Expiration() != 0 && ret.Expiration() < time.Duration(clock.Now(m.ctx).UnixNano()) {
-                ret = nil
-                ok = false
-                delete(m.data.items, key)
+func (m *memcacheImpl) Flush() error {
+        m.data.lock.Lock()
+        defer m.data.lock.Unlock()
+
+        m.data.reset()
+        return nil
+}
+
+func (m *memcacheImpl) Increment(key string, delta int64, initialValue *uint64) (uint64, error) {
+        m.data.lock.RLock()
+        defer m.data.lock.RUnlock()
+
+        now := clock.Now(m.ctx)

[dnj, 2015/08/03 22:37:25]

Do this outside of the lock.

[iannucci, 2015/08/04 01:21:20]

Done.

+
+        cur := uint64(0)
+        if initialValue == nil {
+                curItm, err := m.data.retrieveLocked(now, key)
+                if err != nil {
+                        return 0, err
+                }
+                if len(curItm.value) != 8 {
+                        return 0, errors.New("memcache Increment: got invalid current value")
+                }
+                cur = binary.LittleEndian.Uint64(curItm.value)
+        } else {
+                cur = *initialValue
         }
-        return ret, ok
+        if delta < 0 {
+                if uint64(-delta) > cur {
+                        cur = 0
+                } else {
+                        cur -= uint64(-delta)
+                }
+        } else {
+                cur += uint64(delta)
+        }
+
+        newval := make([]byte, 8)
+        binary.LittleEndian.PutUint64(newval, cur)
+        m.data.setItemLocked(now, m.NewItem(key).SetValue(newval))

[dnj, 2015/08/03 22:37:25]

This isn't safe to do with just an RLock.

[iannucci, 2015/08/04 01:21:20]

oops, typo

+
+        return cur, nil
 }
+
+func (m *memcacheImpl) Stats() (*mc.Statistics, error) {
+        m.data.lock.RLock()
+        defer m.data.lock.RUnlock()
+
+        ret := m.data.stats
+        return &ret, nil
+}