Merge pull request #2671 from Daniel-B-Smith/moar-branches

Remove more branches from KeyInfo comparisons

fdbserver/SkipList.cpp

@@ -87,6 +87,10 @@ struct KeyInfo {
 	  : key(key), begin(begin), write(write), transaction(transaction), pIndex(pIndex) {}
 };
 
+force_inline int extra_ordering(const KeyInfo& ki) {
+	return ki.begin * 2 + (ki.write ^ ki.begin);
+}
+
 // returns true if done with string
 force_inline bool getCharacter(const KeyInfo& ki, int character, int& outputCharacter) {
 	// normal case
@@ -103,7 +107,7 @@ force_inline bool getCharacter(const KeyInfo& ki, int character, int& outputChar
 
 	if (character == ki.key.size() + 1) {
 		// end/begin+read/write relative sorting
-		outputCharacter = ki.begin * 2 + (ki.write ^ ki.begin);
+		outputCharacter = extra_ordering(ki);
 		return false;
 	}
 
@@ -116,18 +120,16 @@ bool operator<(const KeyInfo& lhs, const KeyInfo& rhs) {
 	int c = memcmp(lhs.key.begin(), rhs.key.begin(), i);
 	if (c != 0) return c < 0;
 
-	// SOMEDAY: This is probably not very fast.  Slows D.Sort by ~20% relative to previous (incorrect) version.
-
-	bool lDone, rDone;
-	int lc, rc;
-	while (true) {
-		lDone = getCharacter(lhs, i, lc);
-		rDone = getCharacter(rhs, i, rc);
-		if (lDone && rDone) return false; // equality
-		if (lc < rc) return true;
-		if (lc > rc) return false;
-		i++;
+	// Always sort shorter keys before longer keys.
+	if (lhs.key.size() < rhs.key.size()) {
+		return true;
 	}
+	if (lhs.key.size() > rhs.key.size()) {
+		return false;
+	}
+
+	// When the keys are the same length, use the extra ordering constraint.
+	return extra_ordering(lhs) < extra_ordering(rhs);
 }
 
 bool operator==(const KeyInfo& lhs, const KeyInfo& rhs) {
@@ -926,15 +928,47 @@ void miniConflictSetTest() {
 	printf("miniConflictSetTest complete\n");
 }
 
+void operatorLessThanTest() {
+	{ // Longer strings before shorter strings.
+		KeyInfo a(LiteralStringRef("hello"), /*begin=*/false, /*write=*/true, 0, nullptr);
+		KeyInfo b(LiteralStringRef("hello\0"), /*begin=*/false, /*write=*/false, 0, nullptr);
+		ASSERT(a < b);
+		ASSERT(!(b < a));
+		ASSERT(!(a == b));
+	}
+
+	{ // Reads before writes.
+		KeyInfo a(LiteralStringRef("hello"), /*begin=*/false, /*write=*/false, 0, nullptr);
+		KeyInfo b(LiteralStringRef("hello"), /*begin=*/false, /*write=*/true, 0, nullptr);
+		ASSERT(a < b);
+		ASSERT(!(b < a));
+		ASSERT(!(a == b));
+	}
+
+	{ // Begin reads after writes.
+		KeyInfo a(LiteralStringRef("hello"), /*begin=*/false, /*write=*/true, 0, nullptr);
+		KeyInfo b(LiteralStringRef("hello"), /*begin=*/true, /*write=*/false, 0, nullptr);
+		ASSERT(a < b);
+		ASSERT(!(b < a));
+		ASSERT(!(a == b));
+	}
+
+	{ // Begin writes after writes.
+		KeyInfo a(LiteralStringRef("hello"), /*begin=*/false, /*write=*/true, 0, nullptr);
+		KeyInfo b(LiteralStringRef("hello"), /*begin=*/true, /*write=*/true, 0, nullptr);
+		ASSERT(a < b);
+		ASSERT(!(b < a));
+		ASSERT(!(a == b));
+	}
+}
+
 void skipListTest() {
 	printf("Skip list test\n");
 
-	// A test case that breaks the old operator<
-	// KeyInfo a( LiteralStringRef("hello"), true, false, true, -1 );
-	// KeyInfo b( LiteralStringRef("hello\0"), false, false, false, 0 );
-
 	miniConflictSetTest();
 
+	operatorLessThanTest();
+
 	setAffinity(0);
 
 	double start;