From e760c78ddc9ca493a8c13723dae93f7b12821be3 Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Tue, 21 Nov 2017 09:11:54 +0900 Subject: [PATCH] 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 | 34 +++++++++++++++---- memory/build/mozjemalloc.cpp | 36 +++++++++++--------- memory/build/rb.h | 64 ++++++++++++++++++------------------ 3 files changed, 79 insertions(+), 55 deletions(-) diff --git a/memory/build/Utils.h b/memory/build/Utils.h index d4bbe4ec645c..5adaa98f3d67 100644 --- a/memory/build/Utils.h +++ b/memory/build/Utils.h @@ -19,16 +19,36 @@ struct Log2 : mozilla::tl::CeilingLog2 }; #define LOG2(N) Log2::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 -int +Order +CompareInt(T aValue1, T aValue2) +{ + static_assert(mozilla::IsIntegral::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 +Order CompareAddr(T* aAddr1, T* aAddr2) { - uintptr_t addr1 = reinterpret_cast(aAddr1); - uintptr_t addr2 = reinterpret_cast(aAddr2); - - return (addr1 > addr2) - (addr1 < addr2); + return CompareInt(uintptr_t(aAddr1), uintptr_t(aAddr2)); } // User-defined literals to make constants more legible diff --git a/memory/build/mozjemalloc.cpp b/memory/build/mozjemalloc.cpp index 315471d29b03..7381ce5b0faa 100644 --- a/memory/build/mozjemalloc.cpp +++ b/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(aKey->mAddr); uintptr_t node_addr = reinterpret_cast(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); } }; diff --git a/memory/build/rb.h b/memory/build/rb.h index d0abf4e45270..3b7b140b20be 100644 --- a/memory/build/rb.h +++ b/memory/build/rb.h @@ -47,15 +47,15 @@ // corresponding to a given node with the following signature: // static RedBlackTreeNode& 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 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 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.