From b9fc9e7ce181b854bb0c14ca911bcfce8f58e17a Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Thu, 5 Jul 2018 11:20:51 +0100 Subject: [PATCH] Fix issues identified in review by gdr Copied from Perforce Change: 194430 --- mps/code/cbs.c | 46 +++++++++++++++++++++++--------------------- mps/code/cbs.h | 1 + mps/code/mpmst.h | 22 --------------------- mps/code/node.h | 12 ++++++++++-- mps/code/range.c | 2 +- mps/code/range.h | 6 ++++++ mps/design/cbs.txt | 12 +++++++----- mps/design/range.txt | 12 ++++++------ 8 files changed, 55 insertions(+), 58 deletions(-) diff --git a/mps/code/cbs.c b/mps/code/cbs.c index fe10b90113b..84e055ef784 100644 --- a/mps/code/cbs.c +++ b/mps/code/cbs.c @@ -6,8 +6,8 @@ * .intro: This is a portable implementation of coalescing block * structures. * - * .purpose: CBSs are used to manage potentially unbounded - * collections of memory blocks. + * .purpose: CBSs are used to manage potentially unbounded collections + * of memory blocks. * * .sources: . */ @@ -25,13 +25,13 @@ SRCID(cbs, "$Id$"); #define cbsOfLand(land) PARENT(CBSStruct, landStruct, land) #define cbsSplay(cbs) (&((cbs)->splayTreeStruct)) -#define cbsOfSplay(_splay) PARENT(CBSStruct, splayTreeStruct, _splay) -#define cbsFastBlockOfTree(_tree) \ - PARENT(CBSFastBlockStruct, nodeStruct, NodeOfTree(_tree)) -#define cbsFastBlockNode(_block) (&(_block)->nodeStruct) -#define cbsZonedBlockOfTree(_tree) \ - PARENT(CBSZonedBlockStruct, cbsFastBlockStruct, cbsFastBlockOfTree(_tree)) -#define cbsZonedBlockNode(_block) cbsFastBlockNode(&(_block)->cbsFastBlockStruct) +#define cbsOfSplay(splay) PARENT(CBSStruct, splayTreeStruct, splay) +#define cbsFastBlockOfTree(tree) \ + PARENT(CBSFastBlockStruct, nodeStruct, NodeOfTree(tree)) +#define cbsFastBlockNode(block) (&(block)->nodeStruct) +#define cbsZonedBlockOfTree(tree) \ + PARENT(CBSZonedBlockStruct, cbsFastBlockStruct, cbsFastBlockOfTree(tree)) +#define cbsZonedBlockNode(block) cbsFastBlockNode(&(block)->cbsFastBlockStruct) #define cbsBlockPool(cbs) RVALUE((cbs)->blockPool) @@ -405,22 +405,22 @@ static Res cbsInsert(Range rangeReturn, Land land, Range range) limit = RangeLimit(range); METER_ACC(cbs->treeSearch, cbs->treeSize); - b = SplayTreeNeighbours(&leftSplay, &rightSplay, - cbsSplay(cbs), NodeKeyOfBaseVar(base)); + b = SplayTreeNeighbours(&leftSplay, &rightSplay, cbsSplay(cbs), + NodeKeyOfBaseVar(base)); if (!b) { res = ResFAIL; goto fail; } /* .insert.overlap: The two cases below are not quite symmetrical, - because base was passed into the call to SplayTreeNeighbours(), - but limit was not. So we know that if there is a left neighbour, - then leftBlock's limit <= base (this is ensured by NodeCompare, - which is the comparison method on the tree). But if there is a - right neighbour, all we know is that base < rightBlock's - base. But for the range to fit, we need limit <= rightBlock's - base too. Hence the extra check and the possibility of failure in - the second case. */ + because base was passed into the call to SplayTreeNeighbours(), but + limit was not. So we know that if there is a left neighbour, then + NodeLimit(leftBlock) <= base (this is ensured by NodeCompare, which + is the comparison method on the tree). But if there is a right + neighbour, all we know is that base < NodeBase(rightBlock). But for + the range to fit, we need limit <= NodeBase(rightBlock) too. Hence + the extra check and the possibility of failure in the second + case. */ if (leftSplay == TreeEMPTY) { leftBlock = NULL; @@ -436,7 +436,7 @@ static Res cbsInsert(Range rangeReturn, Land land, Range range) rightMerge = FALSE; } else { rightBlock = NodeOfTree(rightSplay); - if (rightBlock != NULL && limit > NodeLimit(rightBlock)) { + if (rightBlock != NULL && limit > NodeBase(rightBlock)) { /* .insert.overlap */ res = ResFAIL; goto fail; @@ -861,7 +861,7 @@ static Bool cbsFindFirst(Range rangeReturn, Range oldRangeReturn, /* cbsFindInZones -- find a block within a zone set * - * Fins a block of at least the given size that lies entirely within a + * Finds a block of at least the given size that lies entirely within a * zone set. (The first such block, if high is FALSE, or the last, if * high is TRUE.) */ @@ -881,7 +881,8 @@ static Bool cbsTestNodeInZones(SplayTree splay, Tree tree, Node block = NodeOfTree(tree); cbsTestNodeInZonesClosure my = closure; RangeInZoneSet search; - + + AVER_CRITICAL(closure != NULL); UNUSED(splay); search = my->high ? RangeInZoneSetLast : RangeInZoneSetFirst; @@ -898,6 +899,7 @@ static Bool cbsTestTreeInZones(SplayTree splay, Tree tree, CBSZonedBlock zonedBlock = cbsZonedBlockOfTree(tree); cbsTestNodeInZonesClosure my = closure; + AVER_CRITICAL(closure != NULL); UNUSED(splay); return fastBlock->maxSize >= my->size diff --git a/mps/code/cbs.h b/mps/code/cbs.h index 006bbba1362..e38b44607e4 100644 --- a/mps/code/cbs.h +++ b/mps/code/cbs.h @@ -12,6 +12,7 @@ #include "arg.h" #include "mpmtypes.h" #include "mpmst.h" +#include "node.h" #include "range.h" #include "splay.h" diff --git a/mps/code/mpmst.h b/mps/code/mpmst.h index c1c1fc98ff1..5d583a3e213 100644 --- a/mps/code/mpmst.h +++ b/mps/code/mpmst.h @@ -240,28 +240,6 @@ typedef struct SegClassStruct { } SegClassStruct; -/* RangeStruct -- address range - * - * See design.mps.range, range.h, range.c. - */ - -typedef struct RangeStruct { - Addr base; - Addr limit; -} RangeStruct; - - -/* NodeStruct -- address range in a tree - * - * See node.h, node.c. - */ - -typedef struct NodeStruct { - TreeStruct treeStruct; - RangeStruct rangeStruct; -} NodeStruct; - - /* SegStruct -- segment structure * * .seg: Segments are the basic units of protection and tracer activity diff --git a/mps/code/node.h b/mps/code/node.h index 7e9d5d40448..877c95831e7 100644 --- a/mps/code/node.h +++ b/mps/code/node.h @@ -8,6 +8,7 @@ #define node_h #include "mpmtypes.h" +#include "range.h" #include "tree.h" #define NodeTree(node) (&(node)->treeStruct) @@ -29,10 +30,10 @@ extern void NodeFinish(Node node); /* Functions for nodes in trees * - * We pass the ndoe base directly as a TreeKey (void *) assuming that + * We pass the node base directly as a TreeKey (void *) assuming that * Addr can be encoded, and possibly breaking . * On an exotic platform where this isn't true, pass the address of - * base. i.e. add an & + * base: that is, add an &. */ #define NodeKeyOfBaseVar(baseVar) ((TreeKey)(baseVar)) @@ -42,6 +43,13 @@ extern Compare NodeCompare(Tree tree, TreeKey key); extern TreeKey NodeKey(Tree tree); +/* NodeStruct -- address range in a tree */ + +typedef struct NodeStruct { + TreeStruct treeStruct; + RangeStruct rangeStruct; +} NodeStruct; + #endif /* node_h */ /* C. COPYRIGHT AND LICENSE diff --git a/mps/code/range.c b/mps/code/range.c index a9627e39077..a93a4dfe5f6 100644 --- a/mps/code/range.c +++ b/mps/code/range.c @@ -41,7 +41,7 @@ void RangeFinish(Range range) AVERT(Range, range); /* Make range invalid and recognisably so. */ range->limit = (Addr)0; - range->base = (Addr)0xF191583D; + range->base = (Addr)0xF191583D; /* FINISHED */ } Res RangeDescribe(Range range, mps_lib_FILE *stream, Count depth) diff --git a/mps/code/range.h b/mps/code/range.h index 69ba18b28d8..a4a2db3d8a3 100644 --- a/mps/code/range.h +++ b/mps/code/range.h @@ -40,6 +40,12 @@ extern void (RangeSetLimit)(Range range, Addr addr); extern Size (RangeSize)(Range range); extern void RangeCopy(Range to, Range from); +/* RangeStruct -- address range */ + +typedef struct RangeStruct { + Addr base; + Addr limit; +} RangeStruct; #endif /* range_h */ diff --git a/mps/design/cbs.txt b/mps/design/cbs.txt index 20ef30d61e7..c72636c116a 100644 --- a/mps/design/cbs.txt +++ b/mps/design/cbs.txt @@ -111,7 +111,9 @@ following optional keyword arguments: the block descriptor pool automatically extends itself when out of space; if ``FALSE``, the pool returns ``ResLIMIT`` in this case. (This feature is used by the arena to bootstrap its own CBS of free - memory. See design.mps.bootstrap.land.sol.pool.) + memory. See design.mps.bootstrap.land.sol.pool_.) + + .. _design.mps.bootstrap.land.sol.pool: bootstrap#land-sol-pool Limitations @@ -238,10 +240,10 @@ this would make coalescence slightly less eager, by up to _`.future.iterate.and.delete`: It would be possible to provide an implementation for the ``LandIterateAndDelete()`` generic function -using ``TreeTraverseAndDelete``, which calls ``TreeToVine()`` first, +using ``TreeTraverseAndDelete()``, which calls ``TreeToVine()`` first, iterates over the vine (where deletion is straightforward), and then -rebalances the tree. Note that this is little better than using -``SplayFirst`` and ``SplayNext``. +rebalances the tree. Note that this is little better than using +``SplayFirst()`` and ``SplayNext()``. _`.future.lazy-coalesce`: It's long been observed that small blocks are often freed and then reallocated, so that coalescing them is a @@ -299,7 +301,7 @@ Document History Documented new keyword arguments. - 2016-03-27 RB_ Adding cross references to usage. Updating future - with reference to ``TreeTraverseAndDelete``. Adding future idea + with reference to ``TreeTraverseAndDelete()``. Adding future idea about lazy coalescing. .. _RB: http://www.ravenbrook.com/consultants/rb/ diff --git a/mps/design/range.txt b/mps/design/range.txt index f65bb10e744..e7c8ab3390f 100644 --- a/mps/design/range.txt +++ b/mps/design/range.txt @@ -81,14 +81,14 @@ there is a function too.) ``void RangeSetBase(Range range, Addr addr)`` -Set the base of the range. ``addr`` must not be greater than the -range limit. To set them both at once, use ``RangeInit``. (This is +Set the base of the range. ``addr`` must not be greater than the range +limit. To set them both at once, use ``RangeInit()``. (This is implemented as a macro, but there is a function too.) ``void RangeSetLimit(Range range, Addr addr)`` -Set the limit of the range. ``addr`` must not be less than the range -base. To set the both at once, use ``RangeInit``. (This is +Set the limit of the range. ``addr`` must not be less than the range +base. To set the both at once, use ``RangeInit()``. (This is implemented as a macro, but there's a function too.) ``Size RangeSize(Range range)`` @@ -129,8 +129,8 @@ Document history ---------------- - 2013-05-21 GDR_ Created. -- 2014-01-15 GDR_ Added ``RangeContains``. -- 2016-03-27 RB_ Addded ``RangeSetBase`` and ``RangeSetLimit``. +- 2014-01-15 GDR_ Added ``RangeContains()``. +- 2016-03-27 RB_ Addded ``RangeSetBase()`` and ``RangeSetLimit()``. .. _GDR: http://www.ravenbrook.com/consultants/gdr/ .. _RB: http://www.ravenbrook.com/consultants/rb/