From ac75dc2812f692eea527c084620ed93be031c0c9 Mon Sep 17 00:00:00 2001
From: beorn7 <beorn@soundcloud.com>
Date: Fri, 8 May 2015 16:36:46 +0200
Subject: [PATCH] Avoid archive lookup for known mapped FPs.

---
 storage/local/mapper.go      | 38 ++++++++++++++++++------------------
 storage/local/mapper_test.go | 15 ++++++++------
 2 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/storage/local/mapper.go b/storage/local/mapper.go
index 4c863ec38..9e65207cd 100644
--- a/storage/local/mapper.go
+++ b/storage/local/mapper.go
@@ -72,7 +72,25 @@ func (r *fpMapper) mapFP(fp clientmodel.Fingerprint, m clientmodel.Metric) (clie
 		// Collision detected!
 		return r.maybeAddMapping(fp, m)
 	}
-	// Metric is not in memory. Check the archive next.
+	// Metric is not in memory. Before doing the expensive archive lookup,
+	// check if we have a mapping for this metric in place already.
+	r.mtx.RLock()
+	mappedFPs, fpAlreadyMapped := r.mappings[fp]
+	r.mtx.RUnlock()
+	if fpAlreadyMapped {
+		// We indeed have mapped fp historically.
+		ms := metricToUniqueString(m)
+		// fp is locked by the caller, so no further locking of
+		// 'collisions' required (it is specific to fp).
+		mappedFP, ok := mappedFPs[ms]
+		if ok {
+			// Historical mapping found, return the mapped FP.
+			return mappedFP, nil
+		}
+	}
+	// If we are here, FP does not exist in memory and is either not mapped
+	// at all, or existing mappings for FP are not for m. Check if we have
+	// something for FP in the archive.
 	archivedMetric, err := r.p.getArchivedMetric(fp)
 	if err != nil {
 		return fp, err
@@ -86,24 +104,6 @@ func (r *fpMapper) mapFP(fp clientmodel.Fingerprint, m clientmodel.Metric) (clie
 		// Collision detected!
 		return r.maybeAddMapping(fp, m)
 	}
-	// The fingerprint is genuinely new. We might have mapped it
-	// historically, though. so we need to check the collisions map.
-	r.mtx.RLock()
-	mappedFPs, ok := r.mappings[fp]
-	r.mtx.RUnlock()
-	if !ok {
-		// No historical mapping, we are good.
-		return fp, nil
-	}
-	// We indeed have mapped fp historically.
-	ms := metricToUniqueString(m)
-	// fp is locked by the caller, so no further locking of 'collisions'
-	// required (it is specific to fp).
-	mappedFP, ok := mappedFPs[ms]
-	if ok {
-		// Historical mapping found, return the mapped FP.
-		return mappedFP, nil
-	}
 	// As fp does not exist, neither in memory nor in archive, we can safely
 	// keep it unmapped.
 	return fp, nil
diff --git a/storage/local/mapper_test.go b/storage/local/mapper_test.go
index a322f50e9..e85f2e269 100644
--- a/storage/local/mapper_test.go
+++ b/storage/local/mapper_test.go
@@ -367,28 +367,31 @@ func TestFPMapper(t *testing.T) {
 	}
 
 	// To make sure that the mapping layer is not queried if the FP is found
-	// either in sm or in the archive, now put fp1 with cm12 in sm and fp2
-	// with cm22 into archive (which will never happen in practice as only
-	// mapped FPs are put into sm and the archive.
+	// in sm but the mapping layer is queried before going to the archive,
+	// now put fp1 with cm12 in sm and fp2 with cm22 into archive (which
+	// will never happen in practice as only mapped FPs are put into sm and
+	// the archive).
 	sm.put(fp1, &memorySeries{metric: cm12})
 	p.archiveMetric(fp2, cm22, 0, 0)
 	gotFP, err = mapper.mapFP(fp1, cm12)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if wantFP := fp1; gotFP != wantFP {
+	if wantFP := fp1; gotFP != wantFP { // No mapping happened.
 		t.Errorf("got fingerprint %v, want fingerprint %v", gotFP, wantFP)
 	}
 	gotFP, err = mapper.mapFP(fp2, cm22)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if wantFP := fp2; gotFP != wantFP {
+	if wantFP := clientmodel.Fingerprint(3); gotFP != wantFP { // Old mapping still applied.
 		t.Errorf("got fingerprint %v, want fingerprint %v", gotFP, wantFP)
 	}
 
 	// If we now map cm21, we should get a mapping as the collision with the
-	// archived metric is detected.
+	// archived metric is detected. Again, this is a pathological situation
+	// that must never happen in real operations. It's just staged here to
+	// test the expected behavior.
 	gotFP, err = mapper.mapFP(fp2, cm21)
 	if err != nil {
 		t.Fatal(err)