cockroachdb · nvanbenschoten · Nov 22, 2017 · Nov 9, 2017 · Nov 16, 2017
@@ -640,13 +640,11 @@ func TestRangeTransferLease(t *testing.T) {
 
 		// We'd like to verify the timestamp cache's low water mark, but this is
 		// impossible to determine precisely in all cases because it may have
-		// been subsumed by future tscache accesses. However, we know that there
-		// were no tscache accesses since we executed the transfer lease
-		// request, which means that the low water mark will still be equal to
-		// the high water mark. So instead of checking the low water mark, we
-		// make sure that the high water mark is set to the new lease start
-		// time, which is less than the previous lease's expiration time.
-		if highWater := replica1.GetTSCacheHighWater(); highWater != replica1Lease.Start {
+		// been subsumed by future tscache accesses. So instead of checking the
+		// low water mark, we make sure that the high water mark is equal to or
+		// greater than the new lease start time, which is less than the
+		// previous lease's expiration time.
+		if highWater := replica1.GetTSCacheHighWater(); highWater.Less(replica1Lease.Start) {
 			t.Fatalf("expected timestamp cache high water %s, but found %s",
 				replica1Lease.Start, highWater)
 		}

@@ -24,6 +24,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 
 	"github.com/cockroachdb/cockroach/pkg/config"
@@ -37,7 +38,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/pkg/errors"
 )
 
 // AddReplica adds the replica to the store's replica map and to the sorted
@@ -312,8 +312,10 @@ func (r *Replica) CommandSizesLen() int {
 // GetTSCacheHighWater returns the high water mark of the replica's timestamp
 // cache.
 func (r *Replica) GetTSCacheHighWater() hlc.Timestamp {
-	r.store.tsCacheMu.Lock()
-	defer r.store.tsCacheMu.Unlock()
+	if !r.store.tsCacheMu.cache.ThreadSafe() {
+		r.store.tsCacheMu.Lock()
+		defer r.store.tsCacheMu.Unlock()
+	}
 
 	start := roachpb.Key(r.Desc().StartKey)
 	end := roachpb.Key(r.Desc().EndKey)

@@ -1956,9 +1956,13 @@ func (ec *endCmds) done(br *roachpb.BatchResponse, pErr *roachpb.Error, retry pr
 			// Clockless mode: all reads count as writes.
 			creq.Writes, creq.Reads = append(creq.Writes, creq.Reads...), nil
 		}
-		ec.repl.store.tsCacheMu.Lock()
+		if !ec.repl.store.tsCacheMu.cache.ThreadSafe() {
+			ec.repl.store.tsCacheMu.Lock()
+		}
 		ec.repl.store.tsCacheMu.cache.AddRequest(creq)
-		ec.repl.store.tsCacheMu.Unlock()
+		if !ec.repl.store.tsCacheMu.cache.ThreadSafe() {
+			ec.repl.store.tsCacheMu.Unlock()
+		}
 	}
 
 	if fn := ec.repl.store.cfg.TestingKnobs.OnCommandQueueAction; fn != nil {
@@ -2338,8 +2342,10 @@ func (r *Replica) applyTimestampCache(
 
 	// TODO(peter): We only need to hold a write lock during the ExpandRequests
 	// calls. Investigate whether using a RWMutex here reduces lock contention.
-	r.store.tsCacheMu.Lock()
-	defer r.store.tsCacheMu.Unlock()
+	if !r.store.tsCacheMu.cache.ThreadSafe() {
+		r.store.tsCacheMu.Lock()
+		defer r.store.tsCacheMu.Unlock()
+	}
 
 	if ba.Txn != nil {
 		r.store.tsCacheMu.cache.ExpandRequests(span, ba.Txn.Timestamp)

@@ -209,11 +209,15 @@ func (r *Replica) leasePostApply(ctx context.Context, newLease roachpb.Lease) {
 		// lease's expiration but instead use the new lease's start to initialize
 		// the timestamp cache low water.
 		desc := r.Desc()
-		r.store.tsCacheMu.Lock()
+		if !r.store.tsCacheMu.cache.ThreadSafe() {
+			r.store.tsCacheMu.Lock()
+		}
 		for _, keyRange := range makeReplicatedKeyRanges(desc) {
 			r.store.tsCacheMu.cache.SetLowWater(keyRange.start.Key, keyRange.end.Key, newLease.Start)
 		}
-		r.store.tsCacheMu.Unlock()
+		if !r.store.tsCacheMu.cache.ThreadSafe() {
+			r.store.tsCacheMu.Unlock()
+		}
 
 		// Reset the request counts used to make lease placement decisions whenever
 		// starting a new lease.

@@ -1963,8 +1963,10 @@ func TestReplicaUpdateTSCache(t *testing.T) {
 		t.Error(pErr)
 	}
 	// Verify the timestamp cache has rTS=1s and wTS=0s for "a".
-	tc.repl.store.tsCacheMu.Lock()
-	defer tc.repl.store.tsCacheMu.Unlock()
+	if !tc.repl.store.tsCacheMu.cache.ThreadSafe() {
+		tc.repl.store.tsCacheMu.Lock()
+		defer tc.repl.store.tsCacheMu.Unlock()
+	}
 	tc.repl.store.tsCacheMu.cache.ExpandRequests(tc.repl.Desc().RSpan(), hlc.Timestamp{})
 	noID := uuid.UUID{}
 	rTS, rTxnID := tc.repl.store.tsCacheMu.cache.GetMaxRead(roachpb.Key("a"), nil)

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 )
@@ -50,17 +51,31 @@ const MinRetentionWindow = 10 * time.Second
 // instead to determine read-write conflicts. The low water mark is initialized
 // to the current system time plus the maximum clock offset.
 type Cache interface {
+	// ThreadSafe returns whether the Cache implementation is thread-safe and
+	// therefore does not need external locking.
+	//
+	// TODO(nvanbenschoten): We expose this while the treeImpl is still being
+	// used because finer grained locking internally in each method would hurt
+	// performance. Once we switch to the sklImpl by default, we can give
+	// treeImpl its own lock and let it do its own locking.
+	ThreadSafe() bool
+
 	// AddRequest adds the specified request to the cache in an unexpanded state.
+	//
+	// TODO(nvanbenschoten): once we switch to using the sklImpl by default, we
+	// should trim this interface. We can remove AddRequest and  ExpandRequests,
+	// and export Add directly, because we no longer need to group operations
+	// into a single synchronized access. Doing so will hurt the performance of
+	// treeImpl but improve performance of sklImpl. Because of this tradeoff, we
+	// don't want to do this until we switch from using treeImpl by default to
+	// using sklImpl by default.
 	AddRequest(req *Request)
 	// ExpandRequests expands any request that overlaps the specified span and
 	// which is newer than the specified timestamp.
 	ExpandRequests(span roachpb.RSpan, timestamp hlc.Timestamp)
-
 	// SetLowWater sets the low water mark of the cache for the specified span
 	// to the provided timestamp.
 	SetLowWater(start, end roachpb.Key, timestamp hlc.Timestamp)
-	// GlobalLowWater returns the low water mark for the entire cache.
-	GlobalLowWater() hlc.Timestamp
 
 	// GetMaxRead returns the maximum read timestamp which overlaps the interval
 	// spanning from start to end. If that maximum timestamp belongs to a single
@@ -78,13 +93,18 @@ type Cache interface {
 	//
 	// add the specified timestamp to the cache covering the range of keys from
 	// start to end.
-	add(start, end roachpb.Key, timestamp hlc.Timestamp, txnID uuid.UUID, readTSCache bool)
+	add(start, end roachpb.Key, ts hlc.Timestamp, txnID uuid.UUID, readCache bool)
 	// clear clears the cache and resets the low-water mark.
 	clear(lowWater hlc.Timestamp)
+	// getLowWater return the low water mark for the specified cache.
+	getLowWater(readCache bool) hlc.Timestamp
 }
 
 // New returns a new timestamp cache with the supplied hybrid clock.
 func New(clock *hlc.Clock) Cache {
+	if envutil.EnvOrDefaultBool("COCKROACH_USE_SKL_TSCACHE", false) {
+		return newSklImpl(clock)
+	}
 	return newTreeImpl(clock)
 }