From 59334b5e4fac75a343ec7df2af7d176518fa56c9 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 23 Feb 2014 13:54:07 +0000 Subject: [PATCH 01/12] Updating copyright dates. Copied from Perforce Change: 184510 ServerID: perforce.ravenbrook.com --- mps/code/splay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/code/splay.c b/mps/code/splay.c index 6002d0a1702..2779228c9e6 100644 --- a/mps/code/splay.c +++ b/mps/code/splay.c @@ -1,7 +1,7 @@ /* splay.c: SPLAY TREE IMPLEMENTATION * * $Id$ - * Copyright (c) 2001 Ravenbrook Limited. See end of file for license. + * Copyright (c) 2001-2014 Ravenbrook Limited. See end of file for license. * * .purpose: Splay trees are used to manage potentially unbounded * collections of ordered things. @@ -1175,7 +1175,7 @@ Res SplayTreeDescribe(SplayTree splay, mps_lib_FILE *stream, /* C. COPYRIGHT AND LICENSE * - * Copyright (C) 2001-2002 Ravenbrook Limited . + * Copyright (C) 2001-2014 Ravenbrook Limited . * All rights reserved. This is an open source license. Contact * Ravenbrook for commercial licensing options. * From f47100fd09ccd0ca6615dbc29a5b21df25e3b21c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 23 Feb 2014 14:42:23 +0000 Subject: [PATCH 02/12] Permitting tree traversals to abort early. Copied from Perforce Change: 184511 ServerID: perforce.ravenbrook.com --- mps/code/cbs.c | 4 ++-- mps/code/tree.c | 25 +++++++++++++++++-------- mps/code/tree.h | 6 ++++-- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/mps/code/cbs.c b/mps/code/cbs.c index d4363f44475..45fd4aaa665 100644 --- a/mps/code/cbs.c +++ b/mps/code/cbs.c @@ -667,8 +667,8 @@ void CBSIterate(CBS cbs, CBSIterateMethod iterate, closure.iterate = iterate; closure.closureP = closureP; closure.closureS = closureS; - TreeTraverse(SplayTreeRoot(tree), tree->compare, tree->nodeKey, - CBSIterateVisit, &closure, 0); + (void)TreeTraverse(SplayTreeRoot(tree), tree->compare, tree->nodeKey, + CBSIterateVisit, &closure, 0); cbsLeave(cbs); return; diff --git a/mps/code/tree.c b/mps/code/tree.c index 277dbaf3772..59d492987ac 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -155,13 +155,16 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, * * * The tree may not be modified during the traversal, and the traversal - * must complete. TODO: Is there a way to abort early? + * must complete. + * + * TreeTraverse is generally superior if comparisons are cheap. */ -void TreeTraverseMorris(Tree tree, TreeVisitor visit, +Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, void *closureP, Size closureS) { Tree node; + Bool visiting = TRUE; AVER(TreeCheck(tree)); AVER(FUNCHECK(visit)); @@ -170,7 +173,8 @@ void TreeTraverseMorris(Tree tree, TreeVisitor visit, node = tree; while (node != TreeEMPTY) { if (node->left == TreeEMPTY) { - (void)visit(node, closureP, closureS); + if (visiting) + visiting = visit(node, closureP, closureS); node = node->right; } else { Tree pre = node->left; @@ -182,7 +186,10 @@ void TreeTraverseMorris(Tree tree, TreeVisitor visit, } if (pre->right == node) { pre->right = TreeEMPTY; - (void)visit(node, closureP, closureS); + if (visiting) + visiting = visit(node, closureP, closureS); + else if (node == tree) + return FALSE; node = node->right; break; } @@ -190,6 +197,8 @@ void TreeTraverseMorris(Tree tree, TreeVisitor visit, } } } + + return visiting; } @@ -231,7 +240,7 @@ static Tree stepUpLeft(Tree node, Tree *parentIO) return parent; } -void TreeTraverse(Tree tree, +Bool TreeTraverse(Tree tree, TreeCompare compare, TreeKeyMethod key, TreeVisitor visit, void *closureP, Size closureS) @@ -246,7 +255,7 @@ void TreeTraverse(Tree tree, node = tree; if (node == TreeEMPTY) - return; + return TRUE; down: if (TreeHasLeft(node)) { @@ -264,7 +273,7 @@ down: up: if (parent == TreeEMPTY) - return; + return TRUE; if (compare(parent, key(node)) != CompareLESS) { node = stepUpLeft(node, &parent); goto up; @@ -279,7 +288,7 @@ up: abort: if (parent == TreeEMPTY) - return; + return FALSE; if (compare(parent, key(node)) != CompareLESS) node = stepUpLeft(node, &parent); else diff --git a/mps/code/tree.h b/mps/code/tree.h index eaa917dca67..42aa135054c 100644 --- a/mps/code/tree.h +++ b/mps/code/tree.h @@ -2,6 +2,8 @@ * * $Id$ * Copyright (C) 2014 Ravenbrook Limited. See end of file for license. + * + * Simple binary trees, for use as building blocks. */ #ifndef tree_h @@ -77,11 +79,11 @@ extern Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, TreeKey key, TreeCompare compare); typedef Bool TreeVisitor(Tree tree, void *closureP, Size closureS); -extern void TreeTraverse(Tree tree, +extern Bool TreeTraverse(Tree tree, TreeCompare compare, TreeKeyMethod key, TreeVisitor visit, void *closureP, Size closureS); -extern void TreeTraverseMorris(Tree tree, TreeVisitor visit, +extern Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, void *closureP, Size closureS); extern void TreeRotateLeft(Tree *nodeIO); From 4ef9c96411f484e9705c16a3257f6ef3213a733b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 26 Feb 2014 01:39:10 +0000 Subject: [PATCH 03/12] Moving and updating most of the usage documentation from the design document to the source code. design documentation should contain justification of design decisions, not a user manual. Copied from Perforce Change: 184547 ServerID: perforce.ravenbrook.com --- mps/code/splay.c | 165 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 124 insertions(+), 41 deletions(-) diff --git a/mps/code/splay.c b/mps/code/splay.c index 2779228c9e6..a7ed45d2fe5 100644 --- a/mps/code/splay.c +++ b/mps/code/splay.c @@ -4,7 +4,8 @@ * Copyright (c) 2001-2014 Ravenbrook Limited. See end of file for license. * * .purpose: Splay trees are used to manage potentially unbounded - * collections of ordered things. + * collections of ordered things. In the MPS these are usually + * address-ordered memory blocks. * * .source: * @@ -34,6 +35,11 @@ SRCID(splay, "$Id$"); #define SplayHasUpdate(splay) ((splay)->updateNode != SplayTrivUpdate) +/* SplayTreeCheck -- check consistency of SplayTree + * + * See guide.impl.c.adt.check and design.mps.check. + */ + Bool SplayTreeCheck(SplayTree splay) { UNUSED(splay); @@ -46,6 +52,18 @@ Bool SplayTreeCheck(SplayTree splay) } +/* SplayTreeInit -- initialise a splay tree + * + * ``compare`` must provide a total ordering on node keys. + * + * ``nodeKey`` extracts a key from a tree node for passing to ``compare``. + * + * ``updateNode`` will be applied to nodes from bottom to top when the + * tree is restructured in order to maintain client properties (see + * design.mps.splay.prop). If SplayTrivUpdate may be passed, faster + * algorithms are chosen for splaying (FIXME: xref design). + */ + void SplayTreeInit(SplayTree splay, TreeCompare compare, TreeKeyMethod nodeKey, @@ -66,6 +84,14 @@ void SplayTreeInit(SplayTree splay, } +/* SplayTreeFinish -- finish a splay tree + * + * Does not attempt to descend or finish any tree nodes. + * + * TODO: Should probably fail on non-empty tree, so that client code is + * forced to decide what to do about that. + */ + void SplayTreeFinish(SplayTree splay) { AVERT(SplayTree, splay); @@ -218,30 +244,40 @@ static Tree SplayZagZag(Tree middle, Tree *leftLastIO, Tree leftPrev) } +/* SplayState -- the state of splaying between "split" and "assemble" + * + * Splaying is divided into two phases: splitting the tree into three, + * and then assembling a final tree. This allows for optimisation of + * certain operations, the key one being SplayTreeNeighbours, which is + * critical for coalescing memory blocks (see CBSInsert). + * + * Note that SplaySplitDown and SplaySplitRev use the trees slightly + * differently. SplaySplitRev does not provide "left" and "right", and + * "leftLast" and "rightFirst" are pointer-reversed spines. + */ + +typedef struct SplayStateStruct { + Tree middle; /* always non-empty, has the found node at the root */ + Tree left; /* nodes less than search key during split */ + Tree leftLast; /* rightmost node on right spine of "left" */ + Tree right; /* nodes greater than search key during split */ + Tree rightFirst; /* leftmost node on left spine of "right" */ +} SplayStateStruct, *SplayState; + + /* SplaySplitDown -- divide the tree around a key * * Split a tree into three according to a key and a comparison, * splaying nested left and right nodes. Preserves tree ordering. + * This is a top-down splay procedure, and does not use any recursion + * or require any parent pointers (see design.mps.impl.top-down). * * Returns cmp, the relationship of the root of the middle tree to the key, - * and a SplayState: - * middle -- the middle tree, with the nearest match for the key at the top - * leftTop -- a tree of nodes that were less than node - * leftLast -- the greatest node in leftTop (rightmost child) - * rightTop -- a tree of nodes that were greater than node - * rightFirst -- the least node in rightTop (leftmost child) + * and a SplayState. * - * Does *not* maintain client properties. See SplaySplitRev. + * Does *not* call update to maintain client properties. See SplaySplitRev. */ -typedef struct SplayStateStruct { - Tree middle; - Tree left; - Tree leftLast; - Tree right; - Tree rightFirst; -} SplayStateStruct, *SplayState; - static Compare SplaySplitDown(SplayStateStruct *stateReturn, SplayTree splay, TreeKey key, TreeCompare compare) { @@ -590,6 +626,8 @@ static void SplayAssembleRev(SplayTree splay, SplayState state) } +/* SplaySplit -- call SplaySplitDown or SplaySplitRev as appropriate */ + static Compare SplaySplit(SplayStateStruct *stateReturn, SplayTree splay, TreeKey key, TreeCompare compare) { @@ -599,6 +637,9 @@ static Compare SplaySplit(SplayStateStruct *stateReturn, return SplaySplitDown(stateReturn, splay, key, compare); } + +/* SplayAssemble -- call SplayAssembleDown or SplayAssembleRev as appropriate */ + static void SplayAssemble(SplayTree splay, SplayState state) { if (SplayHasUpdate(splay)) @@ -610,8 +651,11 @@ static void SplayAssemble(SplayTree splay, SplayState state) /* SplaySplay -- splay the tree around a given key * - * If the key is not found, splays around an arbitrary neighbour. - * Returns the relationship of the new tree root to the key. + * Uses SplaySplitRev/SplayAssembleRev or SplaySplitDown/SplayAssembleDown + * as appropriate, but also catches the empty tree case and shortcuts + * the common case where the wanted node is already at the root (due + * to a previous splay). The latter shortcut has a significant effect + * on run time. * * See . */ @@ -649,10 +693,13 @@ static Compare SplaySplay(SplayTree splay, TreeKey key, TreeCompare compare) } -/* SplayTreeInsert -- Insert a node into a splay tree +/* SplayTreeInsert -- insert a node into a splay tree * - * See and - * . + * + * This function is used to insert a node into the tree. Splays the + * tree at the node's key. If an attempt is made to insert a node that + * compares ``CompareEQUAL`` to an existing node in the tree, then + * ``FALSE`` will be returned and the node will not be inserted. * * NOTE: It would be possible to use split here, then assemble around * the new node, leaving the neighbour where it was, but it's probably @@ -702,13 +749,17 @@ Bool SplayTreeInsert(SplayTree splay, Tree node) { } -/* SplayTreeDelete -- Delete a node from a splay tree +/* SplayTreeDelete -- delete a node from a splay tree * - * See and - * . + * Delete a node from the tree. If the tree does not contain the given + * node, or the then ``FALSE`` will be returned, and the node will + * not be deleted. The function first splays the tree at the given key. + * The client must not pass a node whose key compares equal to a different + * node in the tree. * - * TODO: Consider using SplaySplit. If the found node has zero - * or one children, then the replacement will be leftLast or rightFirst. + * TODO: If the node has zero or one children, then the replacement + * would be the leftLast or rightFirst after a SplaySplit, and would + * avoid a search for a replacement in more cases. */ Bool SplayTreeDelete(SplayTree splay, Tree node) { @@ -753,8 +804,9 @@ Bool SplayTreeDelete(SplayTree splay, Tree node) { /* SplayTreeFind -- search for a node in a splay tree matching a key * - * See and - * . + * Search the tree for a node that compares ``CompareEQUAL`` to a key + * Splays the tree at the key. Returns ``FALSE`` if there is no such + * node in the tree, otherwise ``*nodeReturn`` will be set to the node. */ Bool SplayTreeFind(Tree *nodeReturn, SplayTree splay, TreeKey key) { @@ -772,7 +824,7 @@ Bool SplayTreeFind(Tree *nodeReturn, SplayTree splay, TreeKey key) { } -/* SplayTreeSuccessor -- Splays a tree at the root's successor +/* SplayTreeSuccessor -- splays a tree at the root's successor * * Must not be called on en empty tree. Successor need not exist, * in which case TreeEMPTY is returned, and the tree is unchanged. @@ -807,13 +859,19 @@ static Tree SplayTreeSuccessor(SplayTree splay) { /* SplayTreeNeighbours * * Search for the two nodes in a splay tree neighbouring a key. + * Splays the tree at the key. ``*leftReturn`` will be the neighbour + * which compares less than the key if such a neighbour exists; otherwise + * it will be ``TreeEMPTY``. ``*rightReturn`` will be the neighbour which + * compares greater than the key if such a neighbour exists; otherwise + * it will be ``TreeEMPTY``. The function returns ``FALSE`` if any node + * in the tree compares ``CompareEQUAL`` with the given key. * * TODO: Change to SplayTreeCoalesce that takes a function that can * direct the deletion of one of the neighbours, since this is a * good moment to do it, avoiding another search and splay. * - * See and - * . + * This implementation uses SplaySplit to find both neighbours in a + * single splay (see design.mps.splay.impl.neighbours). */ Bool SplayTreeNeighbours(Tree *leftReturn, Tree *rightReturn, @@ -872,20 +930,22 @@ Bool SplayTreeNeighbours(Tree *leftReturn, Tree *rightReturn, } -/* SplayTreeFirst, SplayTreeNext -- Iterators +/* SplayTreeFirst, SplayTreeNext -- iterators * * SplayTreeFirst receives a key that must precede all * nodes in the tree. It returns TreeEMPTY if the tree is empty. * Otherwise, it splays the tree to the first node, and returns the - * new root. See . + * new root. * - * SplayTreeNext takes a tree and splays it to the successor of the - * old root, and returns the new root. Returns TreeEMPTY is there are - * no successors. It takes a key for the old root. See - * . + * SplayTreeNext takes a tree and splays it to the successor of a key + * and returns the new root. Returns TreeEMPTY is there are no successors. * - * Iterating over the tree using these functions will leave the tree - * totally unbalanced. Consider using TreeTraverse. + * SplayTreeFirst and SplayTreeNext do not require the tree to remain + * unmodified. + * + * IMPORTANT: Iterating over the tree using these functions will leave + * the tree totally unbalanced, throwing away optimisations of the tree + * shape caused by previous splays. Consider using TreeTraverse instead. */ Tree SplayTreeFirst(SplayTree splay) { @@ -970,6 +1030,21 @@ static Res SplayNodeDescribe(Tree node, mps_lib_FILE *stream, } +/* SplayFindFirstCompare, SplayFindLastCompare -- filtering searches + * + * These are used by SplayFindFirst and SplayFindLast as comparison + * functions to SplaySplit in order to home in on a node using client + * tests. The way to understand them is that the comparison values + * they return have nothing to do with the tree ordering, but are instead + * like commands that tell SplaySplit whether to "go left", "stop", or + * "go right" according to the results of testNode and testTree. + * Since splaying preserves the order of the tree, any tests can be + * applied to navigate to a destination. + * + * In the MPS these are mainly used by the CBS to search for memory + * blocks above a certain size. Their performance is quite critical. + */ + typedef struct SplayFindClosureStruct { SplayTestNodeMethod testNode; SplayTestTreeMethod testTree; @@ -990,6 +1065,8 @@ static Compare SplayFindFirstCompare(Tree node, TreeKey key) AVERT(Tree, node); AVER(key != NULL); + /* Lift closure values into variables so that they aren't aliased by + calls to the test functions. */ closure = (SplayFindClosure)key; closureP = closure->p; closureS = closure->s; @@ -1021,6 +1098,8 @@ static Compare SplayFindLastCompare(Tree node, TreeKey key) AVERT(Tree, node); AVER(key != NULL); + /* Lift closure values into variables so that they aren't aliased by + calls to the test functions. */ closure = (SplayFindClosure)key; closureP = closure->p; closureS = closure->s; @@ -1047,6 +1126,8 @@ static Compare SplayFindLastCompare(Tree node, TreeKey key) * tree that satisfies some property defined by the client. The * property is such that the client can detect, given a sub-tree, * whether that sub-tree contains any nodes satisfying the property. + * If there is no satisfactory node, ``FALSE`` is returned, otherwise + * ``*nodeReturn`` is set to the node. * * The given callbacks testNode and testTree detect this property in * a single node or a sub-tree rooted at a node, and both receive the @@ -1115,14 +1196,16 @@ Bool SplayFindLast(Tree *nodeReturn, SplayTree splay, } -/* SplayNodeRefresh -- Updates the client property that has changed at a node +/* SplayNodeRefresh -- updates the client property that has changed at a node * * This function undertakes to call the client updateNode callback for each * node affected by the change in properties at the given node (which has * the given key) in an appropriate order. * * The function fullfils its job by first splaying at the given node, and - * updating the single node. This may change. + * updating the single node. In the MPS it is used by the CBS during + * coalescing, when the node is likely to be at (or adjacent to) the top + * of the tree anyway. */ void SplayNodeRefresh(SplayTree splay, Tree node) From 347436131e725de8ea21ed16292cf22bd48168da Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 26 Feb 2014 12:59:54 +0000 Subject: [PATCH 04/12] Updating references to not go through unpublished mminfo documents. Copied from Perforce Change: 184549 ServerID: perforce.ravenbrook.com --- mps/design/splay.txt | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/mps/design/splay.txt b/mps/design/splay.txt index 2b69b03c94b..77b573270a4 100644 --- a/mps/design/splay.txt +++ b/mps/design/splay.txt @@ -21,8 +21,8 @@ implementation. _`.readership`: This document is intended for any MM developer. -_`.source`: The primary sources for this design are paper.st85(0) and -paper.sleator96(0). Also as CBS is a client, design.mps.cbs. As +_`.source`: The primary sources for this design are [ST85]_ and +[Sleator96]_. Also as CBS is a client, design.mps.cbs. As PoolMVFF is an indirect client, design.mps.poolmvff(1). Also, as PoolMV2 is an (obsolescent?) indirect client, design.mps.poolmv2. @@ -914,6 +914,18 @@ left and right trees with pointers reversed. This would remove the need for the assembly phase to reverse them. +References +---------- + +.. [ST85] "Self-Adjusting Binary Search Trees"; Daniel Dominic Sleator, + Robert Endre Tarjan; AT&T Bell Laboratories, Murray Hill, NJ; 1985-07; + Journal of the ACM, Vol. 32, Num. 3, pp. 652-686, July 1985; + . + +.. [Sleator96] "Splay Trees"; Daniel Dominic Sleator; CMU, 22/02/96; + CMU 15-211; . + + Document History ---------------- From dc4652b2718f4212b7ec829e0fd5efbc37f70fda Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 26 Feb 2014 13:00:07 +0000 Subject: [PATCH 05/12] Minor tweaks to comments. Copied from Perforce Change: 184550 ServerID: perforce.ravenbrook.com --- mps/code/splay.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/code/splay.c b/mps/code/splay.c index a7ed45d2fe5..a9769335a38 100644 --- a/mps/code/splay.c +++ b/mps/code/splay.c @@ -7,7 +7,7 @@ * collections of ordered things. In the MPS these are usually * address-ordered memory blocks. * - * .source: + * .source: * * .note.stack: It's important that the MPS have a bounded stack * size, and this is a problem for tree algorithms. Basically, @@ -37,7 +37,7 @@ SRCID(splay, "$Id$"); /* SplayTreeCheck -- check consistency of SplayTree * - * See guide.impl.c.adt.check and design.mps.check. + * See guide.impl.c.adt.check and . */ Bool SplayTreeCheck(SplayTree splay) @@ -60,7 +60,7 @@ Bool SplayTreeCheck(SplayTree splay) * * ``updateNode`` will be applied to nodes from bottom to top when the * tree is restructured in order to maintain client properties (see - * design.mps.splay.prop). If SplayTrivUpdate may be passed, faster + * design.mps.splay.prop). If SplayTrivUpdate is be passed, faster * algorithms are chosen for splaying (FIXME: xref design). */ From 65a00f594502bbf753321916e4ec8b10e7f5f904 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 26 Feb 2014 13:23:31 +0000 Subject: [PATCH 06/12] Minor documentation improvements to binary trees. Copied from Perforce Change: 184552 ServerID: perforce.ravenbrook.com --- mps/code/tree.c | 24 ++++++++++++++++++------ mps/code/tree.h | 3 ++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/mps/code/tree.c b/mps/code/tree.c index 59d492987ac..20d03ce6f46 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -2,6 +2,9 @@ * * $Id$ * Copyright (C) 2014 Ravenbrook Limited. See end of file for license. + * + * Simple binary trees with utilities, for use as building blocks. + * Keep it simple, like Rings (see ring.h). */ #include "tree.h" @@ -154,10 +157,11 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, * * * - * The tree may not be modified during the traversal, and the traversal - * must complete. + * The tree may not be accessed or modified during the traversal, and + * the traversal must complete in order to repair the tree. * - * TreeTraverse is generally superior if comparisons are cheap. + * TreeTraverse is generally superior if comparisons are cheap, but + * TreeTraverseMorris does not require any comparison function. */ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, @@ -202,7 +206,13 @@ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, } -/* TreeTraverse -- traverse tree using pointer reversal */ +/* TreeTraverse -- traverse tree using pointer reversal + * + * The tree may not be accessed or modified during the traversal, and + * the traversal must complete in order to repair the tree. + * + * TreeTraverseMorris is an alternative when no cheap comparison is available. + */ static Tree stepDownLeft(Tree node, Tree *parentIO) { @@ -300,7 +310,8 @@ abort: /* TreeRotateLeft -- Rotate right child edge of node * * Rotates node, right child of node, and left child of right - * child of node, leftwards in the order stated. + * child of node, leftwards in the order stated. Preserves tree + * ordering. */ void TreeRotateLeft(Tree *treeIO) @@ -323,7 +334,8 @@ void TreeRotateLeft(Tree *treeIO) /* TreeRotateRight -- Rotate left child edge of node * * Rotates node, left child of node, and right child of left - * child of node, leftwards in the order stated. + * child of node, leftwards in the order stated. Preserves tree + * ordering. */ void TreeRotateRight(Tree *treeIO) { diff --git a/mps/code/tree.h b/mps/code/tree.h index 42aa135054c..1eaebd23932 100644 --- a/mps/code/tree.h +++ b/mps/code/tree.h @@ -3,7 +3,8 @@ * $Id$ * Copyright (C) 2014 Ravenbrook Limited. See end of file for license. * - * Simple binary trees, for use as building blocks. + * Simple binary trees with utilities, for use as building blocks. + * Keep it simple, like Rings (see ring.h). */ #ifndef tree_h From 4e28809e4abdd843c7b7c290f162486ffb887d5d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 7 Mar 2014 15:00:54 +0000 Subject: [PATCH 07/12] Responding to code review. see . Copied from Perforce Change: 184669 ServerID: perforce.ravenbrook.com --- mps/code/cbs.c | 47 +++++++++++++++++++++++++++-------------- mps/code/cbs.h | 4 ++-- mps/code/splay.c | 54 ++++++++++++++++++++++++++++++------------------ 3 files changed, 67 insertions(+), 38 deletions(-) diff --git a/mps/code/cbs.c b/mps/code/cbs.c index 45fd4aaa665..8d200cc09fb 100644 --- a/mps/code/cbs.c +++ b/mps/code/cbs.c @@ -110,6 +110,7 @@ static Compare cbsCompare(Tree node, TreeKey key) CBSBlock cbsBlock; AVER(node != NULL); + AVER(node != TreeEMPTY); base1 = *(Addr *)key; cbsBlock = cbsBlockOfNode(node); @@ -406,7 +407,7 @@ static Res cbsInsertIntoTree(Range rangeReturn, CBS cbs, Range range) * range to fit, we need limit <= rightCBS->base too. Hence the extra * check and the possibility of failure in the second case. */ - if (leftSplay == NULL) { + if (leftSplay == TreeEMPTY) { leftCBS = NULL; leftMerge = FALSE; } else { @@ -415,7 +416,7 @@ static Res cbsInsertIntoTree(Range rangeReturn, CBS cbs, Range range) leftMerge = leftCBS->limit == base; } - if (rightSplay == NULL) { + if (rightSplay == TreeEMPTY) { rightCBS = NULL; rightMerge = FALSE; } else { @@ -595,7 +596,8 @@ static Res cbsBlockDescribe(CBSBlock block, mps_lib_FILE *stream) { Res res; - if (stream == NULL) return ResFAIL; + if (stream == NULL) + return ResFAIL; res = WriteF(stream, "[$P,$P) {$U}", @@ -610,28 +612,39 @@ static Res cbsSplayNodeDescribe(Tree node, mps_lib_FILE *stream) { Res res; - if (node == NULL) return ResFAIL; - if (stream == NULL) return ResFAIL; + if (node == TreeEMPTY) + return ResFAIL; + if (stream == NULL) + return ResFAIL; res = cbsBlockDescribe(cbsBlockOfNode(node), stream); return res; } -/* CBSIterate -- Iterate all blocks in CBS +/* CBSIterate -- iterate over all blocks in CBS + * + * Applies a visitor to all isolated contiguous ranges in a CBS. + * It receives a pointer, ``Size`` closure pair to pass on to the + * visitor function, and an visitor function to invoke on every range + * in address order. If the visitor returns ``FALSE``, then the iteration + * is terminated. + * + * The visitor function may not modify the CBS during the iteration. + * This is because CBSIterate uses TreeTraverse, which does not permit + * modification, for speed and to avoid perturbing the splay tree balance. * - * This is not necessarily efficient. * See . */ typedef struct CBSIterateClosure { CBS cbs; - CBSIterateMethod iterate; + CBSVisitor iterate; void *closureP; Size closureS; } CBSIterateClosure; -static Bool CBSIterateVisit(Tree tree, void *closureP, Size closureS) +static Bool cbsIterateVisit(Tree tree, void *closureP, Size closureS) { CBSIterateClosure *closure = closureP; RangeStruct range; @@ -648,7 +661,7 @@ static Bool CBSIterateVisit(Tree tree, void *closureP, Size closureS) return TRUE; } -void CBSIterate(CBS cbs, CBSIterateMethod iterate, +void CBSIterate(CBS cbs, CBSVisitor visitor, void *closureP, Size closureS) { SplayTree tree; @@ -656,7 +669,7 @@ void CBSIterate(CBS cbs, CBSIterateMethod iterate, AVERT(CBS, cbs); cbsEnter(cbs); - AVER(FUNCHECK(iterate)); + AVER(FUNCHECK(visitor)); tree = treeOfCBS(cbs); /* .splay-iterate.slow: We assume that splay tree iteration does */ @@ -664,11 +677,11 @@ void CBSIterate(CBS cbs, CBSIterateMethod iterate, METER_ACC(cbs->treeSearch, cbs->treeSize); closure.cbs = cbs; - closure.iterate = iterate; + closure.iterate = visitor; closure.closureP = closureP; closure.closureS = closureS; (void)TreeTraverse(SplayTreeRoot(tree), tree->compare, tree->nodeKey, - CBSIterateVisit, &closure, 0); + cbsIterateVisit, &closure, 0); cbsLeave(cbs); return; @@ -840,7 +853,7 @@ Bool CBSFindLargest(Range rangeReturn, Range oldRangeReturn, if (!SplayTreeIsEmpty(treeOfCBS(cbs))) { RangeStruct range; CBSBlock block; - Tree node = NULL; /* suppress "may be used uninitialized" */ + Tree node = TreeEMPTY; /* suppress "may be used uninitialized" */ Size maxSize; maxSize = cbsBlockOfNode(SplayTreeRoot(treeOfCBS(cbs)))->maxSize; @@ -872,8 +885,10 @@ Res CBSDescribe(CBS cbs, mps_lib_FILE *stream) { Res res; - if (!TESTT(CBS, cbs)) return ResFAIL; - if (stream == NULL) return ResFAIL; + if (!TESTT(CBS, cbs)) + return ResFAIL; + if (stream == NULL) + return ResFAIL; res = WriteF(stream, "CBS $P {\n", (WriteFP)cbs, diff --git a/mps/code/cbs.h b/mps/code/cbs.h index 7ef712f6249..51f54541bf1 100644 --- a/mps/code/cbs.h +++ b/mps/code/cbs.h @@ -17,7 +17,7 @@ typedef struct CBSStruct *CBS; -typedef Bool (*CBSIterateMethod)(CBS cbs, Range range, +typedef Bool (*CBSVisitor)(CBS cbs, Range range, void *closureP, Size closureS); @@ -43,7 +43,7 @@ extern void CBSFinish(CBS cbs); extern Res CBSInsert(Range rangeReturn, CBS cbs, Range range); extern Res CBSDelete(Range rangeReturn, CBS cbs, Range range); -extern void CBSIterate(CBS cbs, CBSIterateMethod iterate, +extern void CBSIterate(CBS cbs, CBSVisitor visitor, void *closureP, Size closureS); extern Res CBSDescribe(CBS cbs, mps_lib_FILE *stream); diff --git a/mps/code/splay.c b/mps/code/splay.c index a9769335a38..549451c9a95 100644 --- a/mps/code/splay.c +++ b/mps/code/splay.c @@ -11,7 +11,8 @@ * * .note.stack: It's important that the MPS have a bounded stack * size, and this is a problem for tree algorithms. Basically, - * we have to avoid recursion. FIXME: Reference requirement. + * we have to avoid recursion. TODO: Design documentation for this + * requirement, meanwhile see job003651 and job003640. */ @@ -61,7 +62,8 @@ Bool SplayTreeCheck(SplayTree splay) * ``updateNode`` will be applied to nodes from bottom to top when the * tree is restructured in order to maintain client properties (see * design.mps.splay.prop). If SplayTrivUpdate is be passed, faster - * algorithms are chosen for splaying (FIXME: xref design). + * algorithms are chosen for splaying. Compare SplaySplitDown with + * SplaySplitRev. */ void SplayTreeInit(SplayTree splay, @@ -146,7 +148,7 @@ static Compare compareGreater(Tree tree, TreeKey key) * * A debugging utility to recursively update the client property of * a subtree. May not be used in production MPS because it has - * indefinite stack usage. FIXME: Ref to requirement. + * indefinite stack usage. See .note.stack. */ void SplayDebugUpdate(SplayTree splay, Tree tree) @@ -164,7 +166,7 @@ void SplayDebugUpdate(SplayTree splay, Tree tree) /* SplayZig -- move to left child, prepending to right tree * * Link the top node of the middle tree into the left child of the - * right tree, then step to the left child. + * right tree, then step to the left child. Returns new middle. * * See . * @@ -192,13 +194,14 @@ static Tree SplayZig(Tree middle, Tree *rightFirstIO, Tree *rightNextReturn) /* SplayZigZig -- move to left child, rotating on on to right tree * * Rotate the top node of the middle tree over the left child of the - * right tree, then step to the left child. + * right tree, then step to the left child, completing a splay "zig zig" + * after an initial SplayZig. Returns new middle. * * middle rightNext middle rightNext * B E A E * / \ / \ => / \ - * A C D F right B F - * right \ + * A C D F rightFirst B F + * rightFirst \ * D * / * C @@ -288,6 +291,7 @@ static Compare SplaySplitDown(SplayStateStruct *stateReturn, AVERT(SplayTree, splay); AVER(FUNCHECK(compare)); AVER(!SplayTreeIsEmpty(splay)); + AVER(!SplayHasUpdate(splay)); TreeInit(&sentinel); leftLast = &sentinel; @@ -388,6 +392,7 @@ static void SplayAssembleDown(SplayTree splay, SplayState state) { AVERT(SplayTree, splay); AVER(state->middle != TreeEMPTY); + AVER(!SplayHasUpdate(splay)); if (state->left != TreeEMPTY) { AVER_CRITICAL(state->leftLast != TreeEMPTY); @@ -657,6 +662,13 @@ static void SplayAssemble(SplayTree splay, SplayState state) * to a previous splay). The latter shortcut has a significant effect * on run time. * + * If a matching node is found, it is splayed to the root and the function + * returns CompareEQUAL, or if the tree is empty, will also return + * CompareEQUAL. Otherwise, CompareGREATER or CompareLESS is returned + * meaning either the key is greater or less than the new root. In this + * case the new root is the last node visited which is either the closest + * node left or the closest node right of the key. + * * See . */ @@ -752,10 +764,10 @@ Bool SplayTreeInsert(SplayTree splay, Tree node) { /* SplayTreeDelete -- delete a node from a splay tree * * Delete a node from the tree. If the tree does not contain the given - * node, or the then ``FALSE`` will be returned, and the node will - * not be deleted. The function first splays the tree at the given key. - * The client must not pass a node whose key compares equal to a different - * node in the tree. + * node then ``FALSE`` will be returned. The client must not pass a + * node whose key compares equal to a different node in the tree. + * + * The function first splays the tree at the given key. * * TODO: If the node has zero or one children, then the replacement * would be the leftLast or rightFirst after a SplaySplit, and would @@ -777,10 +789,10 @@ Bool SplayTreeDelete(SplayTree splay, Tree node) { if (cmp != CompareEQUAL) { return FALSE; - } else if (TreeLeft(node) == TreeEMPTY) { + } else if (!TreeHasLeft(node)) { SplayTreeSetRoot(splay, TreeRight(node)); TreeClearRight(node); - } else if (TreeRight(node) == TreeEMPTY) { + } else if (!TreeHasRight(node)) { SplayTreeSetRoot(splay, TreeLeft(node)); TreeClearLeft(node); } else { @@ -791,7 +803,7 @@ Bool SplayTreeDelete(SplayTree splay, Tree node) { SplaySplay(splay, NULL, compareGreater); leftLast = SplayTreeRoot(splay); AVER(leftLast != TreeEMPTY); - AVER(TreeRight(leftLast) == TreeEMPTY); + AVER(!TreeHasRight(leftLast)); TreeSetRight(leftLast, rightHalf); splay->updateNode(splay, leftLast); } @@ -1004,7 +1016,7 @@ static Res SplayNodeDescribe(Tree node, mps_lib_FILE *stream, res = WriteF(stream, "( ", NULL); if (res != ResOK) return res; - if (TreeLeft(node) != TreeEMPTY) { + if (TreeHasLeft(node)) { res = SplayNodeDescribe(TreeLeft(node), stream, nodeDescribe); if (res != ResOK) return res; @@ -1015,7 +1027,7 @@ static Res SplayNodeDescribe(Tree node, mps_lib_FILE *stream, res = (*nodeDescribe)(node, stream); if (res != ResOK) return res; - if (TreeRight(node) != TreeEMPTY) { + if (TreeHasRight(node)) { res = WriteF(stream, " \\ ", NULL); if (res != ResOK) return res; @@ -1140,6 +1152,7 @@ Bool SplayFindFirst(Tree *nodeReturn, SplayTree splay, void *closureP, Size closureS) { SplayFindClosureStruct closureStruct; + Compare cmp; AVER(nodeReturn != NULL); AVERT(SplayTree, splay); @@ -1156,8 +1169,8 @@ Bool SplayFindFirst(Tree *nodeReturn, SplayTree splay, closureStruct.testTree = testTree; closureStruct.splay = splay; - if (SplaySplay(splay, &closureStruct, SplayFindFirstCompare) != CompareEQUAL) - return FALSE; + cmp = SplaySplay(splay, &closureStruct, SplayFindFirstCompare); + AVER(cmp == CompareEQUAL); *nodeReturn = SplayTreeRoot(splay); return TRUE; @@ -1172,6 +1185,7 @@ Bool SplayFindLast(Tree *nodeReturn, SplayTree splay, void *closureP, Size closureS) { SplayFindClosureStruct closureStruct; + Compare cmp; AVER(nodeReturn != NULL); AVERT(SplayTree, splay); @@ -1188,8 +1202,8 @@ Bool SplayFindLast(Tree *nodeReturn, SplayTree splay, closureStruct.testTree = testTree; closureStruct.splay = splay; - if (SplaySplay(splay, &closureStruct, SplayFindLastCompare) != CompareEQUAL) - return FALSE; + cmp = SplaySplay(splay, &closureStruct, SplayFindLastCompare); + AVER(cmp == CompareEQUAL); *nodeReturn = SplayTreeRoot(splay); return TRUE; From 00420bfce52ed7737c35a265338d8398f77f3369 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 11 Mar 2014 14:46:47 +0000 Subject: [PATCH 08/12] Fixing freebsd build. Copied from Perforce Change: 184719 ServerID: perforce.ravenbrook.com --- mps/code/comm.gmk | 2 +- mps/code/commpre.nmk | 2 +- mps/code/tree.c | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/mps/code/comm.gmk b/mps/code/comm.gmk index 1fca1af7b0f..0b48b3678ed 100644 --- a/mps/code/comm.gmk +++ b/mps/code/comm.gmk @@ -169,7 +169,7 @@ MPMCOMMON = abq.c arena.c arenacl.c arenavm.c arg.c boot.c bt.c \ freelist.c global.c ld.c locus.c message.c meter.c mpm.c mpsi.c \ pool.c poolabs.c poolmfs.c poolmrg.c poolmv.c protocol.c range.c \ ref.c reserv.c ring.c root.c sa.c sac.c seg.c shield.c splay.c ss.c \ - table.c trace.c traceanc.c tract.c walk.c + table.c trace.c traceanc.c tract.c tree.c walk.c MPM = $(MPMCOMMON) $(MPMPF) diff --git a/mps/code/commpre.nmk b/mps/code/commpre.nmk index a78eee3ef22..bb8501814ab 100644 --- a/mps/code/commpre.nmk +++ b/mps/code/commpre.nmk @@ -96,7 +96,7 @@ MPMCOMMON = \ \ \ \ - + PLINTH = AMC = AMS = diff --git a/mps/code/tree.c b/mps/code/tree.c index 20d03ce6f46..8e14b8d89b3 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -8,6 +8,7 @@ */ #include "tree.h" +#include "mpm.h" SRCID(tree, "$Id$"); From 1a37468bcad9079a108c70d8fb647e29b94ce3da Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 11 Mar 2014 15:22:21 +0000 Subject: [PATCH 09/12] Fixing windows build. Copied from Perforce Change: 184721 ServerID: perforce.ravenbrook.com --- mps/code/commpre.nmk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/code/commpre.nmk b/mps/code/commpre.nmk index bb8501814ab..11273ed3966 100644 --- a/mps/code/commpre.nmk +++ b/mps/code/commpre.nmk @@ -95,7 +95,7 @@ MPMCOMMON = \ \ \ \ -
\ +
\ PLINTH = AMC = From 1f2cf6aa56a6ec685e531149a649d3712ac577d5 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 11 Mar 2014 13:48:31 +0000 Subject: [PATCH 10/12] Responding to code review . Updating Splay Tree design document to be reasonably consistent with the current code. Disabling tree functions not currently in use. Copied from Perforce Change: 184723 ServerID: perforce.ravenbrook.com --- mps/code/cbs.h | 2 +- mps/code/tree.c | 24 +++++++- mps/design/splay.txt | 136 +++++++++++++++++-------------------------- 3 files changed, 78 insertions(+), 84 deletions(-) diff --git a/mps/code/cbs.h b/mps/code/cbs.h index 51f54541bf1..f49081a258e 100644 --- a/mps/code/cbs.h +++ b/mps/code/cbs.h @@ -18,7 +18,7 @@ typedef struct CBSStruct *CBS; typedef Bool (*CBSVisitor)(CBS cbs, Range range, - void *closureP, Size closureS); + void *closureP, Size closureS); #define CBSSig ((Sig)0x519CB599) /* SIGnature CBS */ diff --git a/mps/code/tree.c b/mps/code/tree.c index 8e14b8d89b3..7db5bc8f9ee 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -61,6 +61,8 @@ Count TreeDebugCount(Tree tree, TreeCompare compare, TreeKeyMethod key) } +#if 0 /* This code is not currently in use in the MPS */ + /* TreeFind -- search for a node matching the key * * If a matching node is found, sets *treeReturn to that node and returns @@ -95,6 +97,7 @@ Compare TreeFind(Tree *treeReturn, Tree root, TreeKey key, TreeCompare compare) return cmp; case CompareGREATER: node = node->right; + break; default: NOTREACHED; *treeReturn = NULL; @@ -155,14 +158,20 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, /* TreeTraverseMorris -- traverse tree in constant space, n log n time - * - * * * The tree may not be accessed or modified during the traversal, and * the traversal must complete in order to repair the tree. * + * The visitor should return FALSE to terminate the traversal early, + * in which case FALSE is returned. + * * TreeTraverse is generally superior if comparisons are cheap, but * TreeTraverseMorris does not require any comparison function. + * + * + * + * Joseph M. Morris (1979). "Traversing Binary Trees Simply and Cheaply". + * Information Processing Letters 9:5 pp. 197–200. */ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, @@ -206,12 +215,17 @@ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, return visiting; } +#endif /* not currently in use */ + /* TreeTraverse -- traverse tree using pointer reversal * * The tree may not be accessed or modified during the traversal, and * the traversal must complete in order to repair the tree. * + * The visitor should return FALSE to terminate the traversal early, + * in which case FALSE is returned. + * * TreeTraverseMorris is an alternative when no cheap comparison is available. */ @@ -409,6 +423,9 @@ Tree TreeReverseRightSpine(Tree tree) } +#if 0 /* This code is currently not in use in the MPS */ + + /* TreeToVine -- unbalance a tree into a single right spine */ Count TreeToVine(Tree *link) @@ -462,6 +479,9 @@ void TreeBalance(Tree *treeIO) } +#endif /* not currently in use in the MPS */ + + /* C. COPYRIGHT AND LICENSE * * Copyright (C) 2014 Ravenbrook Limited . diff --git a/mps/design/splay.txt b/mps/design/splay.txt index 77b573270a4..8eb1a91c2a8 100644 --- a/mps/design/splay.txt +++ b/mps/design/splay.txt @@ -153,22 +153,23 @@ the root of the splay tree. It is intended that the `.usage.client-tree`_ for an example). No convenience functions are provided for allocation or deallocation. -``typedef struct SplayNodeStruct SplayNodeStruct`` -``typedef struct SplayNodeStruct *SplayNode`` +``typedef struct TreeStruct TreeStruct`` +``typedef struct TreeStruct *Tree`` -_`.type.splay.node`: ``SplayNode`` is the type of a node of the splay -tree. ``SplayNodeStruct`` contains no fields to store the key +_`.type.splay.node`: ``Tree`` is the type of a binary tree, used as the +representation of the nodes of the splay tree. +``Tree`` contains no fields to store the key associated with the node, or the client property. Again, it is -intended that the ``SplayNodeStruct`` can be embedded in another +intended that the ``TreeStruct`` can be embedded in another structure, and that this is how the association will be made (see `.usage.client-node`_ for an example). No convenience functions are provided for allocation or deallocation. -``typedef Compare (*SplayCompareMethod)(void *key, SplayNode node)`` +``typedef Compare (*TreeCompare)(Tree tree, TreeKey key)`` _`.type.splay.compare.method`: A function of type -``SplayCompareMethod`` is required to compare ``key`` with the key the -client associates with that splay tree node ``node``, and return the +``TreeCompare`` is required to compare ``key`` with the key the +client associates with that splay tree node ``tree``, and return the appropriate Compare value (see `.usage.compare`_ for an example). The function compares a key with a node, rather than a pair of keys or nodes as might seem more obvious. This is because the details of the @@ -176,7 +177,7 @@ mapping between nodes and keys is left to the client (see `.type.splay.node`_), and the splaying operations compare keys with nodes (see `.impl.splay`_). -``typedef Res (*SplayNodeDescribeMethod)(SplayNode node, mps_lib_FILE *stream)`` +``typedef Res (*SplayNodeDescribeMethod)(Tree tree, mps_lib_FILE *stream)`` _`.type.splay.node.describe.method`: A function of type ``SplayNodeDescribeMethod`` is required to write (via ``WriteF()``) a @@ -184,7 +185,7 @@ client-oriented representation of the splay node. The output should be non-empty, short, and without return characters. This is provided for debugging purposes only. -``typedef Bool (*SplayTestNodeMethod)(SplayTree tree, SplayNode node, void *closureP, unsigned long closureS)`` +``typedef Bool (*SplayTestNodeMethod)(SplayTree splay, Tree tree, void *closureP, unsigned long closureS)`` _`.type.splay.test.node.method`: A function of type ``SplayTestNodeMethod`` required to determine whether the node itself @@ -193,7 +194,7 @@ meets some client determined property (see `.prop`_ and ``closureS`` describe the environment for the function (see `.function.splay.find.first`_ and `.function.splay.find.last`_). -``typedef Bool (*SplayTestTreeMethod)(SplayTree tree, SplayNode node, void *closureP, unsigned long closureS)`` +``typedef Bool (*SplayTestTreeMethod)(SplayTree splay, Tree tree, void *closureP, unsigned long closureS)`` _`.type.splay.test.tree.method`: A function of type ``SplayTestTreeMethod`` is required to determine whether any of the @@ -206,14 +207,13 @@ return ``TRUE``. Parameters ``closureP`` and ``closureS`` describe the environment for the function (see `.function.splay.find.first`_ and `.function.splay.find.last`_). -``typedef void (*SplayUpdateNodeMethod)(SplayTree tree, SplayNode node, SplayNode leftChild, SplayNode rightChild)`` +``typedef void (*SplayUpdateNodeMethod)(SplayTree splay, Tree tree)`` _`.type.splay.update.node.method`: A function of type ``SplayUpdateNodeMethod`` is required to update any client datastructures associated with a node to maintain some client determined property (see `.prop`_) given that the children of the node -have changed. If the node does not have one or both children, then -``NULL`` will be passed as the relevant parameter. (See +have changed. (See `.usage.callback`_ for an example) @@ -228,19 +228,19 @@ methods invoked by the splay module (`.type.splay.compare.method`_, `.type.splay.update.node.method`_) do not call functions of the splay module. -``Bool SplayTreeCheck(SplayTree tree)`` +``Bool SplayTreeCheck(SplayTree splay)`` _`.function.splay.tree.check`: This is a check function for the SplayTree type (see guide.impl.c.adt.method.check & design.mps.check(0)). -``Bool SplayNodeCheck(SplayNode node)`` +``Bool SplayNodeCheck(Tree tree)`` _`.function.splay.node.check`: This is a check function for the -``SplayNode`` type (see guide.impl.c.adt.method.check & +``Tree`` type (see guide.impl.c.adt.method.check & design.mps.check(0)). -``void SplayTreeInit(SplayTree tree, SplayCompareMethod compare, SplayUpdateNodeMethod updateNode)`` +``void SplayTreeInit(SplayTree splay, SplayCompareMethod compare, SplayUpdateNodeMethod updateNode)`` _`.function.splay.tree.init`: This function initialises a ``SplayTree`` (see guide.impl.c.adt.method.init). It requires a @@ -253,35 +253,16 @@ to date when the tree structure changes; the value need to maintain client properties. (See `.usage.initialization`_ for an example use). -``void SplayTreeFinish(SplayTree tree)`` +``void SplayTreeFinish(SplayTree splay)`` _`.function.splay.tree.finish`: This function clears the fields of a ``SplayTree`` (see guide.impl.c.adt.method.finish). Note that it does -not attempt to finish or deallocate any associated ``SplayNode`` +not attempt to finish or deallocate any associated ``Tree`` objects; clients wishing to destroy a non-empty ``SplayTree`` must first explicitly descend the tree and call ``SplayNodeFinish()`` on each node from the bottom up. -``void SplayNodeInit(SplayNode node)`` - -_`.function.splay.node.init`: This function initialises a -``SplayNode`` (see guide.impl.c.adt.method.init). - -``void SplayNodeFinish(SplayNode node)`` - -_`.function.splay.node.finish`: This function clears the fields of a -``SplayNode`` (see guide.impl.c.adt.method.finish). Note that it does -not attempt to finish or deallocate any referenced ``SplayNode`` -objects (see.function.splay.tree.finish). - -``Bool SplayRoot(SplayNode *nodeReturn, SplayTree tree)`` - -_`.function.splay.root`: This function returns the root node of the -tree, if any (see `.req.root`_). If the tree is empty, ``FALSE`` is -returned and ``*nodeReturn`` is not changed. Otherwise, ``TRUE`` is -returned and ``*nodeReturn`` is set to the root. - -``Bool SplayTreeInsert(SplayTree tree, SplayNode node, void *key)`` +``Bool SplayTreeInsert(SplayTree splay, Tree tree, void *key)`` _`.function.splay.tree.insert`: This function is used to insert into a splay tree a new node which is associated with the supplied key (see @@ -290,7 +271,7 @@ made to insert a node that compares ``CompareEQUAL`` to an existing node in the tree, then ``FALSE`` will be returned and the node will not be inserted. (See `.usage.insert`_ for an example use). -``Bool SplayTreeDelete(SplayTree tree, SplayNode node, void *key)`` +``Bool SplayTreeDelete(SplayTree splay, Tree tree, void *key)`` _`.function.splay.tree.delete`: This function is used to delete from a splay tree a node which is associated with the supplied key (see @@ -300,7 +281,7 @@ given node does not compare ``CompareEQUAL`` with the given key, then function first splays the tree at the given key. (See `.usage.delete`_ for an example use). -``Bool SplayTreeFind(SplayNode *nodeReturn, SplayTree tree, void *key)`` +``Bool SplayTreeFind(Tree *nodeReturn, SplayTree splay, void *key)`` _`.function.splay.tree.search`: This function searches the splay tree for a node that compares ``CompareEQUAL`` to the given key (see @@ -308,26 +289,26 @@ for a node that compares ``CompareEQUAL`` to the given key (see if there is no such node in the tree, otherwise ``*nodeReturn`` will be set to the node. -``Bool SplayTreeNeighbours(SplayNode *leftReturn, SplayNode *rightReturn, SplayTree tree, void *key)`` +``Bool SplayTreeNeighbours(Tree *leftReturn, Tree *rightReturn, SplayTree splay, void *key)`` _`.function.splay.tree.neighbours`: This function searches a splay tree for the two nodes that are the neighbours of the given key (see `.req.neighbours`_). It splays the tree at the key. ``*leftReturn`` will be the neighbour which compares less than the key if such a -neighbour exists; otherwise it will be ``NULL``. ``*rightReturn`` will +neighbour exists; otherwise it will be ``TreeEMPTY``. ``*rightReturn`` will be the neighbour which compares greater than the key if such a -neighbour exists; otherwise it will be ``NULL``. The function returns +neighbour exists; otherwise it will be ``TreeEMPTY``. The function returns ``FALSE`` if any node in the tree compares ``CompareEQUAL`` with the given key. (See `.usage.insert`_ for an example use). -``SplayNode SplayTreeFirst(SplayTree tree, void *zeroKey)`` +``Tree SplayTreeFirst(SplayTree splay, void *zeroKey)`` _`.function.splay.tree.first`: This function splays the tree at the first node, and returns that node (see `.req.iterate`_). The supplied key should compare ``CompareLESS`` with all nodes in the tree. It will -return ``NULL`` if the tree has no nodes. +return ``TreeEMPTY`` if the tree has no nodes. -``SplayNode SplayTreeNext(SplayTree tree, SplayNode oldNode, void *oldKey)`` +``Tree SplayTreeNext(SplayTree splay, Tree oldNode, void *oldKey)`` _`.function.splay.tree.next`: This function receives a node and key and returns the successor node to that node (see `.req.iterate`_). @@ -342,14 +323,14 @@ tree was previously beneficially balanced for a small working set of accesses, then this local optimization will be lost. (see `.future.parent`_). -``Res SplayTreeDescribe(SplayTree tree, mps_lib_FILE *stream, SplayNodeDescribeMethod nodeDescribe)`` +``Res SplayTreeDescribe(SplayTree splay, mps_lib_FILE *stream, SplayNodeDescribeMethod nodeDescribe)`` _`.function.splay.tree.describe`: This function prints (using ``WriteF``) to the stream a textual representation of the given splay tree, using ``nodeDescribe`` to print client-oriented representations of the nodes (see `.req.debug`_). -``Bool SplayFindFirst(SplayNode *nodeReturn, SplayTree tree, SplayTestNodeMethod testNode, SplayTestTreeMethod testTree, void *closureP, unsigned long closureS)`` +``Bool SplayFindFirst(Tree *nodeReturn, SplayTree splay, SplayTestNodeMethod testNode, SplayTestTreeMethod testTree, void *closureP, unsigned long closureS)`` _`.function.splay.find.first`: ``SplayFindFirst()`` finds the first node in the tree that satisfies some client property (as determined by the @@ -360,7 +341,7 @@ closure environments. If there is no satisfactory node, then ``FALSE`` is returned, otherwise ``*nodeReturn`` is set to the node. (See `.usage.delete`_ for an example use). -``Bool SplayFindFirst(SplayNode *nodeReturn, SplayTree tree, SplayTestNodeMethod testNode, SplayTestTreeMethod testTree, void *closureP, unsigned long closureS)`` +``Bool SplayFindFirst(Tree *nodeReturn, SplayTree splay, SplayTestNodeMethod testNode, SplayTestTreeMethod testTree, void *closureP, unsigned long closureS)`` _`.function.splay.find.last`: ``SplayFindLast()`` finds the last node in the tree that satisfies some client property (as determined by the @@ -370,7 +351,7 @@ the ``testNode`` and ``testTree`` methods which may use the values as closure environments. If there is no satisfactory node, then ``FALSE`` is returned, otherwise ``*nodeReturn`` is set to the node. -``void SplayNodeRefresh(SplayTree tree, SplayNode node, void *key)`` +``void SplayNodeRefresh(SplayTree splay, Tree tree, void *key)`` _`.function.splay.node.refresh`: ``SplayNodeRefresh()`` must be called whenever the client property (see `.prop`_) at a node changes (see @@ -462,10 +443,10 @@ _`.usage.client-tree`: Tree structure to embed a ``SplayTree`` (see /* no obvious client fields for this simple example */ } FreeTreeStruct; -_`.usage.client-node`: Node structure to embed a SplayNode (see `.type.splay.node`_):: +_`.usage.client-node`: Node structure to embed a Tree (see `.type.splay.node`_):: typedef struct FreeBlockStruct { - SplayNodeStruct splayNode; /* embedded splay node */ + TreeStruct treeStruct; /* embedded splay node */ Addr base; /* base address of block is also the key */ Size size; /* size of block is also the client property */ Size maxSize; /* cached value for maximum size in subtree */ @@ -474,26 +455,25 @@ _`.usage.client-node`: Node structure to embed a SplayNode (see `.type.splay.nod _`.usage.callback`: updateNode callback method (see `.type.splay.update.node.method`_):: - void FreeBlockUpdateNode(SplayTree tree, SplayNode node, - SplayNode leftChild, SplayNode rightChild) + void FreeBlockUpdateNode(SplayTree splay, Tree tree) { /* Compute the maximum size of any block in this subtree. */ /* The value to cache is the maximum of the size of this block, */ /* the cached value for the left subtree (if any) and the cached */ /* value of the right subtree (if any) */ - FreeBlock freeNode = FreeBlockOfSplayNode(node); + FreeBlock freeNode = FreeBlockOfSplayNode(tree); Size maxSize = freeNode.size; - if(leftChild != NULL) { - FreeBlock leftNode = FreeBlockOfSplayNode(leftChild); + if (TreeHasLeft(tree)) { + FreeBlock leftNode = FreeBlockOfSplayNode(TreeLeft(tree)); if(leftNode.maxSize > maxSize) maxSize = leftNode->maxSize; } - if(rightChild != NULL) { - FreeBlock rightNode = FreeBlockOfSplayNode(rightChild); + if (TreeHasRight(tree)) { + FreeBlock rightNode = FreeBlockOfSplayNode(TreeRight(tree)); if(rightNode.maxSize > maxSize) maxSize = rightNode->maxSize; } @@ -503,9 +483,9 @@ _`.usage.callback`: updateNode callback method (see _`.usage.compare`: Comparison function (see `.type.splay.compare.method`_):: - Compare FreeBlockCompare(void *key, SplayNode node) { + Compare FreeBlockCompare(Tree tree, TreeKey key) { Addr base1, base2, limit2; - FreeBlock freeNode = FreeBlockOfSplayNode(node); + FreeBlock freeNode = FreeBlockOfSplayNode(tree); base1 = (Addr *)key; base2 = freeNode->base; @@ -522,27 +502,27 @@ _`.usage.compare`: Comparison function (see `.type.splay.compare.method`_):: _`.usage.test.tree`: Test tree function (see `.type.splay.test.tree.method`_):: - Bool FreeBlockTestTree(SplayTree tree, SplayNode node + Bool FreeBlockTestTree(SplayTree splay, Tree tree void *closureP, unsigned long closureS) { /* Closure environment has wanted size as value of closureS. */ /* Look at the cached value for the node to see if any */ /* blocks in the subtree are big enough. */ Size size = (Size)closureS; - FreeBlock freeNode = FreeBlockOfSplayNode(node); + FreeBlock freeNode = FreeBlockOfSplayNode(tree); return freeNode->maxSize >= size; } _`.usage.test.node`: Test node function (see `.type.splay.test.node.method`_):: - Bool FreeBlockTestNode(SplayTree tree, SplayNode node + Bool FreeBlockTestNode(SplayTree splay, Tree tree void *closureP, unsigned long closureS) { /* Closure environment has wanted size as value of closureS. */ /* Look at the size of the node to see if is big enough. */ Size size = (Size)closureS; - FreeBlock freeNode = FreeBlockOfSplayNode(node); + FreeBlock freeNode = FreeBlockOfSplayNode(tree); return freeNode->size >= size; } @@ -559,7 +539,7 @@ tree, merging it with an existing block if possible:: void FreeTreeInsert(FreeTree tree, Addr base, Addr limit) { SplayTree splayTree = &tree->splayTree; - SplayNode leftNeighbour, rightNeighbour; + Tree leftNeighbour, rightNeighbour; void *key = (void *)base; /* use the base of the block as the key */ Res res; @@ -570,7 +550,7 @@ tree, merging it with an existing block if possible:: /* Look to see if the neighbours are contiguous. */ - if (leftNeighbour != NULL && + if (leftNeighbour != TreeEMPTY && FreeBlockLimitOfSplayNode(leftNeighbour) == base) { /* Inserted block is contiguous with left neighbour, so merge it. */ /* The client housekeeping is left as an exercise to the reader. */ @@ -578,7 +558,7 @@ tree, merging it with an existing block if possible:: /* property of the splay node. See `.function.splay.node.refresh`_ */ SplayNodeRefresh(tree, leftNeighbour, key); - } else if (rightNeighbour != NULL && + } else if (rightNeighbour != TreeEMPTY && FreeBlockBaseOfSplayNode(rightNeighbour) == limit) { /* Inserted block is contiguous with right neighbour, so merge it. */ /* The client housekeeping is left as an exercise to the reader. */ @@ -607,7 +587,7 @@ block:: Bool FreeTreeAllocate(Addr *baseReturn, Size *sizeReturn, FreeTree tree, Size size) { SplayTree splayTree = &tree->splayTree; - SplayNode splayNode; + Tree splayNode; Bool found; /* look for the first node of at least the given size. */ @@ -878,7 +858,7 @@ protocol error. The cases it doesn't handle will result in undefined behaviour and probably cause an ``AVER`` to fire. These are: _`.error.bad-pointer`: Passing an invalid pointer in place of a -``SplayTree`` or ``SplayNode``. +``SplayTree`` or ``Tree``. _`.error.bad-compare`: Initialising a ``SplayTree`` with a compare function that is not a valid compare function, or which doesn't @@ -893,10 +873,6 @@ _`.error.out-of-stack`: Stack exhaustion under ``SplayTreeDescribe()``. Future ------ -_`.future.tree`: It would be possible to split the splay tree module -into two: one that implements binary trees; and one that implements -splay trees on top of a binary tree. - _`.future.parent`: The iterator could be made more efficient (in an amortized sense) if it didn't splay at each node. To implement this (whilst meeting `.req.stack`_) we really need parent pointers from the @@ -908,11 +884,6 @@ and right-child, and the right-sibling/parent between right-sibling and parent. One could either use the comparator to make these distinctions, or steal some bits from the pointers. -_`.future.reverse`: The assembly phase could be made more efficient if -the link left and link right operations were modified to add to the -left and right trees with pointers reversed. This would remove the -need for the assembly phase to reverse them. - References ---------- @@ -942,6 +913,9 @@ Document History - 2014-02-22 RB_ Fixing abuses of Res and ResFAIL. +- 2014-03-11 RB_ Updating in response to code review. Removing + .future.tree and .future.reverse, both now implemented. + .. _RB: http://www.ravenbrook.com/consultants/rb/ .. _GDR: http://www.ravenbrook.com/consultants/gdr/ From 16e07988063866d0f95e02808b1c61caa5c18a4c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 11 Mar 2014 14:37:10 +0000 Subject: [PATCH 11/12] Responding to code review . Using TREE_ELT where appropriate. Using AVERT consistently on Tree, but see job003691. Copied from Perforce Change: 184724 ServerID: perforce.ravenbrook.com --- mps/code/cbs.c | 4 ++-- mps/code/dbgpool.c | 2 +- mps/code/tree.c | 18 +++++++++--------- mps/code/tree.h | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/mps/code/cbs.c b/mps/code/cbs.c index 8d200cc09fb..dfe98369eb0 100644 --- a/mps/code/cbs.c +++ b/mps/code/cbs.c @@ -34,8 +34,8 @@ typedef struct CBSBlockStruct { #define CBSBlockSize(block) AddrOffset((block)->base, (block)->limit) -#define cbsOfTree(_tree) PARENT(CBSStruct, tree, (_tree)) -#define cbsBlockOfNode(_node) PARENT(CBSBlockStruct, node, (_node)) +#define cbsOfTree(_tree) TREE_ELT(CBS, tree, _tree) +#define cbsBlockOfNode(_node) PARENT(CBSBlockStruct, node, _node) #define treeOfCBS(cbs) (&((cbs)->tree)) #define nodeOfCBSBlock(block) (&((block)->node)) #define keyOfCBSBlock(block) (&((block)->base)) diff --git a/mps/code/dbgpool.c b/mps/code/dbgpool.c index 9d7c6509f12..be109970b5f 100644 --- a/mps/code/dbgpool.c +++ b/mps/code/dbgpool.c @@ -27,7 +27,7 @@ typedef struct tagStruct { } tagStruct; #define TagTree(tag) (&(tag)->treeStruct) -#define TagOfTree(tree) PARENT(tagStruct, treeStruct, tree) +#define TagOfTree(tree) TREE_ELT(tag, treeStruct, tree) typedef tagStruct *Tag; diff --git a/mps/code/tree.c b/mps/code/tree.c index 7db5bc8f9ee..b9f6454b21c 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -78,7 +78,7 @@ Compare TreeFind(Tree *treeReturn, Tree root, TreeKey key, TreeCompare compare) Tree node, parent; Compare cmp = CompareEQUAL; - AVER(TreeCheck(root)); + AVERT(Tree, root); AVER(treeReturn != NULL); AVER(FUNCHECK(compare)); /* key is arbitrary */ @@ -125,7 +125,7 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, Compare cmp; AVER(treeReturn != NULL); - AVER(TreeCheck(root)); + AVER(Tree, root); AVER(TreeCheckLeaf(node)); AVER(FUNCHECK(compare)); /* key is arbitrary */ @@ -149,7 +149,7 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, default: NOTREACHED; *treeReturn = NULL; - return cmp; + return FALSE; } *treeReturn = root; @@ -180,7 +180,7 @@ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, Tree node; Bool visiting = TRUE; - AVER(TreeCheck(tree)); + AVERT(Tree, tree); AVER(FUNCHECK(visit)); /* closureP, closureS arbitrary */ @@ -272,7 +272,7 @@ Bool TreeTraverse(Tree tree, { Tree parent, node; - AVER(TreeCheck(tree)); + AVERT(Tree, tree); AVER(FUNCHECK(visit)); /* closureP, closureS arbitrary */ @@ -373,8 +373,8 @@ void TreeRotateRight(Tree *treeIO) { * * Descends the left spine of a tree, updating each node's left child * to point to its parent instead. The root's left child is set to - * TreeEMPTY. Returns the leftmost child in *leftReturn, or TreeEMPTY - * if the tree was empty. + * TreeEMPTY. Returns the leftmost child, or TreeEMPTY if the tree + * was empty. */ Tree TreeReverseLeftSpine(Tree tree) @@ -400,8 +400,8 @@ Tree TreeReverseLeftSpine(Tree tree) * * Descends the right spine of a tree, updating each node's right child * to point to its parent instead. The root's right child is set to - * TreeEMPTY. Returns the rightmost child in *rightReturn, or TreeEMPTY - * if the tree was empty. + * TreeEMPTY. Returns the rightmost child or TreeEMPTY if the tree + * was empty. */ Tree TreeReverseRightSpine(Tree tree) diff --git a/mps/code/tree.h b/mps/code/tree.h index 1eaebd23932..63e6370a922 100644 --- a/mps/code/tree.h +++ b/mps/code/tree.h @@ -34,13 +34,13 @@ extern Count TreeDebugCount(Tree tree, TreeCompare compare, TreeKeyMethod key); AVER(_tree != NULL); \ _tree->left = TreeEMPTY; \ _tree->right = TreeEMPTY; \ - AVER(TreeCheck(_tree)); \ + AVERT(Tree, _tree); \ END #define TreeFinish(tree) \ BEGIN \ Tree _tree = (tree); \ - AVER(TreeCheckLeaf(_tree)); \ + AVERT(Tree, _tree); \ END #define TREE_ELT(type, field, node) \ From 9d7b1ff29e91e1bc483f980419761ca72c89146b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 11 Mar 2014 15:46:23 +0000 Subject: [PATCH 12/12] Improving comments in response to code review suggestions . Copied from Perforce Change: 184725 ServerID: perforce.ravenbrook.com --- mps/code/tree.c | 13 +++++++++++-- mps/code/tree.h | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/mps/code/tree.c b/mps/code/tree.c index b9f6454b21c..9e19ef6edf9 100644 --- a/mps/code/tree.c +++ b/mps/code/tree.c @@ -5,6 +5,14 @@ * * Simple binary trees with utilities, for use as building blocks. * Keep it simple, like Rings (see ring.h). + * + * The performance requirements on tree implementation will depend on + * how each individual function is applied in the MPS. + * + * .note.stack: It's important that the MPS have a bounded stack + * size, and this is a problem for tree algorithms. Basically, + * we have to avoid recursion. TODO: Design documentation for this + * requirement, meanwhile see job003651 and job003640. */ #include "tree.h" @@ -38,6 +46,7 @@ Bool TreeCheckLeaf(Tree tree) * This function may be called from a debugger or temporarily inserted * during development to check a tree's integrity. It may not be called * from the production MPS because it uses indefinite stack depth. + * See .note.stack. */ static Count TreeDebugCountBetween(Tree node, @@ -157,7 +166,7 @@ Bool TreeInsert(Tree *treeReturn, Tree root, Tree node, } -/* TreeTraverseMorris -- traverse tree in constant space, n log n time +/* TreeTraverseMorris -- traverse tree inorder in constant space * * The tree may not be accessed or modified during the traversal, and * the traversal must complete in order to repair the tree. @@ -218,7 +227,7 @@ Bool TreeTraverseMorris(Tree tree, TreeVisitor visit, #endif /* not currently in use */ -/* TreeTraverse -- traverse tree using pointer reversal +/* TreeTraverse -- traverse tree in-order using pointer reversal * * The tree may not be accessed or modified during the traversal, and * the traversal must complete in order to repair the tree. diff --git a/mps/code/tree.h b/mps/code/tree.h index 63e6370a922..69ee841d3c3 100644 --- a/mps/code/tree.h +++ b/mps/code/tree.h @@ -13,15 +13,42 @@ #include "check.h" #include "mpmtypes.h" + +/* TreeStruct -- binary tree structure + * + * The tree structure is used in a field in other structures in order + * to link them together in a binary tree. + */ + typedef struct TreeStruct *Tree; typedef struct TreeStruct { Tree left, right; } TreeStruct; + +/* TreeKey and TreeCompare -- ordered binary trees + * + * Binary trees are almost always ordered, and these types provide the + * abstraction for ordering. A TreeCompare method returns whether a key + * is less than, equal to, or greater than the key in a tree node. A + * TreeKeyMethod extracts a key from a node, depending on how TreeStruct + * is embedded within its parent structure. + */ + typedef void *TreeKey; typedef Compare (*TreeCompare)(Tree tree, TreeKey key); typedef TreeKey (*TreeKeyMethod)(Tree tree); + +/* TreeEMPTY -- the empty tree + * + * TreeEMPTY is the tree with no nodes, and hence unable to satisfy its + * olfactory senses. Empty trees should not be represented with NULL, + * as this is ambiguous. However, TreeEMPTY is in fact a null pointer for + * performance. To check whether you have it right, try temporarily + * defining TreeEMPTY to (Tree)1 or similar. + */ + #define TreeEMPTY ((Tree)0) extern Bool TreeCheck(Tree tree);