Bug 1419217 - Change comparison functions for red-black trees. r=njn

- This makes all of them return an enum, quite similar to rust.
- This makes all of them derive from a single implementation of an
  integer comparison.
- This implements that integer comparison in terms of simple operations,
  rather than "smart" computation, which turns out to allow compilers
  to do better optimizations.
  See https://blogs.msdn.microsoft.com/oldnewthing/20171117-00/?p=97416

--HG--
extra : rebase_source : 89710d7954f445d43e6da55aaf171b1f57dce5fc

memory/build/Utils.h

@@ -19,16 +19,36 @@ struct Log2 : mozilla::tl::CeilingLog2<N>
 };
 #define LOG2(N) Log2<N>::value
 
-// Compare two addresses. Returns whether the first address is smaller (-1),
-// equal (0) or greater (1) than the second address.
+enum class Order
+{
+  eLess = -1,
+  eEqual = 0,
+  eGreater = 1,
+};
+
+// Compare two integers. Returns whether the first integer is Less,
+// Equal or Greater than the second integer.
 template<typename T>
-int
+Order
+CompareInt(T aValue1, T aValue2)
+{
+  static_assert(mozilla::IsIntegral<T>::value, "Type must be integral");
+  if (aValue1 < aValue2) {
+    return Order::eLess;
+  }
+  if (aValue1 > aValue2) {
+    return Order::eGreater;
+  }
+  return Order::eEqual;
+}
+
+// Compare two addresses. Returns whether the first address is Less,
+// Equal or Greater than the second address.
+template<typename T>
+Order
 CompareAddr(T* aAddr1, T* aAddr2)
 {
-  uintptr_t addr1 = reinterpret_cast<uintptr_t>(aAddr1);
-  uintptr_t addr2 = reinterpret_cast<uintptr_t>(aAddr2);
-
-  return (addr1 > addr2) - (addr1 < addr2);
+  return CompareInt(uintptr_t(aAddr1), uintptr_t(aAddr2));
 }
 
 // User-defined literals to make constants more legible

memory/build/mozjemalloc.cpp

@@ -645,10 +645,11 @@ struct ExtentTreeSzTrait
     return aThis->mLinkBySize;
   }
 
-  static inline int Compare(extent_node_t* aNode, extent_node_t* aOther)
+  static inline Order Compare(extent_node_t* aNode, extent_node_t* aOther)
   {
-    int ret = (aNode->mSize > aOther->mSize) - (aNode->mSize < aOther->mSize);
-    return ret ? ret : CompareAddr(aNode->mAddr, aOther->mAddr);
+    Order ret = CompareInt(aNode->mSize, aOther->mSize);
+    return (ret != Order::eEqual) ? ret
+                                  : CompareAddr(aNode->mAddr, aOther->mAddr);
   }
 };
 
@@ -659,7 +660,7 @@ struct ExtentTreeTrait
     return aThis->mLinkByAddr;
   }
 
-  static inline int Compare(extent_node_t* aNode, extent_node_t* aOther)
+  static inline Order Compare(extent_node_t* aNode, extent_node_t* aOther)
   {
     return CompareAddr(aNode->mAddr, aOther->mAddr);
   }
@@ -667,7 +668,7 @@ struct ExtentTreeTrait
 
 struct ExtentTreeBoundsTrait : public ExtentTreeTrait
 {
-  static inline int Compare(extent_node_t* aKey, extent_node_t* aNode)
+  static inline Order Compare(extent_node_t* aKey, extent_node_t* aNode)
   {
     uintptr_t key_addr = reinterpret_cast<uintptr_t>(aKey->mAddr);
     uintptr_t node_addr = reinterpret_cast<uintptr_t>(aNode->mAddr);
@@ -675,10 +676,10 @@ struct ExtentTreeBoundsTrait : public ExtentTreeTrait
 
     // Is aKey within aNode?
     if (node_addr <= key_addr && key_addr < node_addr + node_size) {
-      return 0;
+      return Order::eEqual;
     }
 
-    return (key_addr > node_addr) - (key_addr < node_addr);
+    return CompareAddr(aKey->mAddr, aNode->mAddr);
   }
 };
 
@@ -793,7 +794,8 @@ struct ArenaChunkMapLink
 
 struct ArenaRunTreeTrait : public ArenaChunkMapLink
 {
-  static inline int Compare(arena_chunk_map_t* aNode, arena_chunk_map_t* aOther)
+  static inline Order Compare(arena_chunk_map_t* aNode,
+                              arena_chunk_map_t* aOther)
   {
     MOZ_ASSERT(aNode);
     MOZ_ASSERT(aOther);
@@ -803,14 +805,16 @@ struct ArenaRunTreeTrait : public ArenaChunkMapLink
 
 struct ArenaAvailTreeTrait : public ArenaChunkMapLink
 {
-  static inline int Compare(arena_chunk_map_t* aNode, arena_chunk_map_t* aOther)
+  static inline Order Compare(arena_chunk_map_t* aNode,
+                              arena_chunk_map_t* aOther)
   {
     size_t size1 = aNode->bits & ~gPageSizeMask;
     size_t size2 = aOther->bits & ~gPageSizeMask;
-    int ret = (size1 > size2) - (size1 < size2);
-    return ret ? ret
-               : CompareAddr((aNode->bits & CHUNK_MAP_KEY) ? nullptr : aNode,
-                             aOther);
+    Order ret = CompareInt(size1, size2);
+    return (ret != Order::eEqual)
+             ? ret
+             : CompareAddr((aNode->bits & CHUNK_MAP_KEY) ? nullptr : aNode,
+                           aOther);
   }
 };
 
@@ -821,7 +825,7 @@ struct ArenaDirtyChunkTrait
     return aThis->link_dirty;
   }
 
-  static inline int Compare(arena_chunk_t* aNode, arena_chunk_t* aOther)
+  static inline Order Compare(arena_chunk_t* aNode, arena_chunk_t* aOther)
   {
     MOZ_ASSERT(aNode);
     MOZ_ASSERT(aOther);
@@ -1097,11 +1101,11 @@ struct ArenaTreeTrait
     return aThis->mLink;
   }
 
-  static inline int Compare(arena_t* aNode, arena_t* aOther)
+  static inline Order Compare(arena_t* aNode, arena_t* aOther)
   {
     MOZ_ASSERT(aNode);
     MOZ_ASSERT(aOther);
-    return (aNode->mId > aOther->mId) - (aNode->mId < aOther->mId);
+    return CompareInt(aNode->mId, aOther->mId);
   }
 };
 

memory/build/rb.h

@@ -47,15 +47,15 @@
 //    corresponding to a given node with the following signature:
 //      static RedBlackTreeNode<T>& GetTreeNode(T*)
 //  - a Compare function with the following signature:
-//      static int Compare(T* aNode, T* aOther)
-//                            ^^^^^
-//                         or aKey
+//      static Order Compare(T* aNode, T* aOther)
+//                              ^^^^^
+//                           or aKey
 //
 // Interpretation of comparision function return values:
 //
-//   -1 : aNode <  aOther
-//    0 : aNode == aOther
-//    1 : aNode >  aOther
+//   Order::eLess: aNode <  aOther
+//   Order::eEqual: aNode == aOther
+//   Order::eGreater: aNode >  aOther
 //
 // In all cases, the aNode or aKey argument is the first argument to the
 // comparison function, which makes it possible to write comparison functions
@@ -205,11 +205,11 @@ private:
       MOZ_ASSERT(rbp_n_t);
       ret = nullptr;
       while (true) {
-        int rbp_n_cmp = Trait::Compare(aNode, rbp_n_t);
-        if (rbp_n_cmp < 0) {
+        Order rbp_n_cmp = Trait::Compare(aNode, rbp_n_t);
+        if (rbp_n_cmp == Order::eLess) {
           ret = rbp_n_t;
           rbp_n_t = rbp_n_t->Left();
-        } else if (rbp_n_cmp > 0) {
+        } else if (rbp_n_cmp == Order::eGreater) {
           rbp_n_t = rbp_n_t->Right();
         } else {
           break;
@@ -230,10 +230,10 @@ private:
       MOZ_ASSERT(rbp_p_t);
       ret = nullptr;
       while (true) {
-        int rbp_p_cmp = Trait::Compare(aNode, rbp_p_t);
-        if (rbp_p_cmp < 0) {
+        Order rbp_p_cmp = Trait::Compare(aNode, rbp_p_t);
+        if (rbp_p_cmp == Order::eLess) {
           rbp_p_t = rbp_p_t->Left();
-        } else if (rbp_p_cmp > 0) {
+        } else if (rbp_p_cmp == Order::eGreater) {
           ret = rbp_p_t;
           rbp_p_t = rbp_p_t->Right();
         } else {
@@ -248,9 +248,9 @@ private:
   TreeNode* Search(TreeNode* aKey)
   {
     TreeNode* ret = mRoot;
-    int rbp_se_cmp;
-    while (ret && (rbp_se_cmp = Trait::Compare(aKey, ret)) != 0) {
-      if (rbp_se_cmp < 0) {
+    Order rbp_se_cmp;
+    while (ret && (rbp_se_cmp = Trait::Compare(aKey, ret)) != Order::eEqual) {
+      if (rbp_se_cmp == Order::eLess) {
         ret = ret->Left();
       } else {
         ret = ret->Right();
@@ -264,11 +264,11 @@ private:
     TreeNode* ret = nullptr;
     TreeNode* rbp_ns_t = mRoot;
     while (rbp_ns_t) {
-      int rbp_ns_cmp = Trait::Compare(aKey, rbp_ns_t);
-      if (rbp_ns_cmp < 0) {
+      Order rbp_ns_cmp = Trait::Compare(aKey, rbp_ns_t);
+      if (rbp_ns_cmp == Order::eLess) {
         ret = rbp_ns_t;
         rbp_ns_t = rbp_ns_t->Left();
-      } else if (rbp_ns_cmp > 0) {
+      } else if (rbp_ns_cmp == Order::eGreater) {
         rbp_ns_t = rbp_ns_t->Right();
       } else {
         ret = rbp_ns_t;
@@ -284,7 +284,7 @@ private:
     // AlignedStorage2 to avoid running the TreeNode base class constructor.
     mozilla::AlignedStorage2<TreeNode> rbp_i_s;
     TreeNode *rbp_i_g, *rbp_i_p, *rbp_i_c, *rbp_i_t, *rbp_i_u;
-    int rbp_i_cmp = 0;
+    Order rbp_i_cmp = Order::eEqual;
     rbp_i_g = nullptr;
     rbp_i_p = rbp_i_s.addr();
     rbp_i_p->SetLeft(mRoot);
@@ -325,10 +325,10 @@ private:
           }
           rbp_i_p = rbp_i_u;
           rbp_i_cmp = Trait::Compare(aNode, rbp_i_p);
-          if (rbp_i_cmp < 0) {
+          if (rbp_i_cmp == Order::eLess) {
             rbp_i_c = rbp_i_p->Left();
           } else {
-            MOZ_ASSERT(rbp_i_cmp > 0);
+            MOZ_ASSERT(rbp_i_cmp == Order::eGreater);
             rbp_i_c = rbp_i_p->Right();
           }
           continue;
@@ -337,10 +337,10 @@ private:
       rbp_i_g = rbp_i_p;
       rbp_i_p = rbp_i_c;
       rbp_i_cmp = Trait::Compare(aNode, rbp_i_c);
-      if (rbp_i_cmp < 0) {
+      if (rbp_i_cmp == Order::eLess) {
         rbp_i_c = rbp_i_c->Left();
       } else {
-        MOZ_ASSERT(rbp_i_cmp > 0);
+        MOZ_ASSERT(rbp_i_cmp == Order::eGreater);
         rbp_i_c = rbp_i_c->Right();
       }
     }
@@ -348,7 +348,7 @@ private:
     aNode->SetLeft(nullptr);
     aNode->SetRight(nullptr);
     aNode->SetColor(NodeColor::Red);
-    if (rbp_i_cmp > 0) {
+    if (rbp_i_cmp == Order::eGreater) {
       rbp_i_p->SetRight(aNode);
       rbp_i_t = LeanLeft(rbp_i_p);
       if (rbp_i_g->Left() == rbp_i_p) {
@@ -370,7 +370,7 @@ private:
     // AlignedStorage2 to avoid running the TreeNode base class constructor.
     mozilla::AlignedStorage2<TreeNode> rbp_r_s;
     TreeNode *rbp_r_p, *rbp_r_c, *rbp_r_xp, *rbp_r_t, *rbp_r_u;
-    int rbp_r_cmp;
+    Order rbp_r_cmp;
     rbp_r_p = rbp_r_s.addr();
     rbp_r_p->SetLeft(mRoot);
     rbp_r_p->SetRight(nullptr);
@@ -383,7 +383,7 @@ private:
     // is reached. Handle the root specially though, since there may
     // be no way to convert it from a 2-node to a 3-node.
     rbp_r_cmp = Trait::Compare(aNode, rbp_r_c);
-    if (rbp_r_cmp < 0) {
+    if (rbp_r_cmp == Order::eLess) {
       rbp_r_t = rbp_r_c->Left();
       rbp_r_u = rbp_r_t ? rbp_r_t->Left() : nullptr;
       if ((!rbp_r_t || rbp_r_t->IsBlack()) &&
@@ -399,7 +399,7 @@ private:
         rbp_r_c = rbp_r_c->Left();
       }
     } else {
-      if (rbp_r_cmp == 0) {
+      if (rbp_r_cmp == Order::eEqual) {
         MOZ_ASSERT(aNode == rbp_r_c);
         if (!rbp_r_c->Right()) {
           // Delete root node (which is also a leaf node).
@@ -416,10 +416,10 @@ private:
           // successor. Record enough information to do the
           // swap later. rbp_r_xp is the aNode's parent.
           rbp_r_xp = rbp_r_p;
-          rbp_r_cmp = 1; // Note that deletion is incomplete.
+          rbp_r_cmp = Order::eGreater; // Note that deletion is incomplete.
         }
       }
-      if (rbp_r_cmp == 1) {
+      if (rbp_r_cmp == Order::eGreater) {
         if (rbp_r_c->Right() && (!rbp_r_c->Right()->Left() ||
                                  rbp_r_c->Right()->Left()->IsBlack())) {
           rbp_r_t = rbp_r_c->Left();
@@ -449,11 +449,11 @@ private:
         }
       }
     }
-    if (rbp_r_cmp != 0) {
+    if (rbp_r_cmp != Order::eEqual) {
       while (true) {
         MOZ_ASSERT(rbp_r_p);
         rbp_r_cmp = Trait::Compare(aNode, rbp_r_c);
-        if (rbp_r_cmp < 0) {
+        if (rbp_r_cmp == Order::eLess) {
           rbp_r_t = rbp_r_c->Left();
           if (!rbp_r_t) {
             // rbp_r_c now refers to the successor node to
@@ -492,7 +492,7 @@ private:
         } else {
           // Check whether to delete this node (it has to be
           // the correct node and a leaf node).
-          if (rbp_r_cmp == 0) {
+          if (rbp_r_cmp == Order::eEqual) {
             MOZ_ASSERT(aNode == rbp_r_c);
             if (!rbp_r_c->Right()) {
               // Delete leaf node.