From 09d0dfc481d8cabc3d8a64ff59d0133a5eba6ead Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Thu, 20 Jun 2013 16:25:08 +0100 Subject: [PATCH] Document the intention behind mps_ld_isstale rather than the implementation: it captures the intention of testing whether a particular dependency is stale (even though the implementation in fact tests whether any dependency is stale). add new function mps_ls_isstale_any for expressing the intention of testing whether any dependencey is stale. the two functions have the same implementation at present, but the separation gives us the freedom to return a more precise result from mps_ld_isstale in future. Copied from Perforce Change: 182816 ServerID: perforce.ravenbrook.com --- mps/code/ld.c | 23 ++++++-- mps/code/mpm.h | 1 + mps/code/mps.h | 1 + mps/code/mpsi.c | 9 +++ mps/code/mpsicv.c | 1 + mps/manual/source/glossary/l.rst | 4 +- mps/manual/source/guide/advanced.rst | 24 +++----- mps/manual/source/topic/location.rst | 85 ++++++++++++++++++---------- 8 files changed, 94 insertions(+), 54 deletions(-) diff --git a/mps/code/ld.c b/mps/code/ld.c index 766bfe5d823..e74c90ec792 100644 --- a/mps/code/ld.c +++ b/mps/code/ld.c @@ -1,7 +1,7 @@ /* ld.c: LOCATION DEPENDENCY IMPLEMENTATION * * $Id$ - * Copyright (c) 2001 Ravenbrook Limited. See end of file for license. + * Copyright (c) 2001-2013 Ravenbrook Limited. See end of file for license. * * .def: A location dependency records the fact that the bit-patterns * of some references will be used directly (most likely for @@ -102,7 +102,7 @@ void LDAdd(mps_ld_t ld, Arena arena, Addr addr) } -/* LDIsStale -- check whether a dependency is stale +/* LDIsStaleAny -- check whether any dependency is stale * * .stale.thread-safe: This function is thread safe. It will return a * correct (but possibly conservative) answer regardless of the number @@ -122,12 +122,10 @@ void LDAdd(mps_ld_t ld, Arena arena, Addr addr) * .stale.old: Otherwise, if the dependency is older than the length * of the history, check it against all movement that has ever occured. */ -Bool LDIsStale(mps_ld_t ld, Arena arena, Addr addr) +Bool LDIsStaleAny(mps_ld_t ld, Arena arena) { RefSet rs; - UNUSED(addr); - AVER(ld->_epoch <= arena->epoch); /* AVERT(Arena, arena) -- .stale.thread-safe */ @@ -148,6 +146,19 @@ Bool LDIsStale(mps_ld_t ld, Arena arena, Addr addr) } +/* LDIsStaleAny -- check whether a particular dependency is stale + * + * .stale.conservative: In fact we just ignore the address and test if + * any dependency is stale. This is conservatively correct (no false + * negatives) but provides a hook for future improvement. + */ +Bool LDIsStale(mps_ld_t ld, Arena arena, Addr addr) +{ + UNUSED(addr); + return LDIsStaleAny(ld, arena); +} + + /* LDAge -- age the arena by adding a moved set * * This stores the fact that a set of references has changed in @@ -212,7 +223,7 @@ void LDMerge(mps_ld_t ld, Arena arena, mps_ld_t from) /* C. COPYRIGHT AND LICENSE * - * Copyright (C) 2001-2002 Ravenbrook Limited . + * Copyright (C) 2001-2013 Ravenbrook Limited . * All rights reserved. This is an open source license. Contact * Ravenbrook for commercial licensing options. * diff --git a/mps/code/mpm.h b/mps/code/mpm.h index 6bdabf4cf62..37957a1bcc1 100644 --- a/mps/code/mpm.h +++ b/mps/code/mpm.h @@ -907,6 +907,7 @@ extern Res MutatorFaultContextScan(ScanState ss, MutatorFaultContext mfc); extern void LDReset(mps_ld_t ld, Arena arena); extern void LDAdd(mps_ld_t ld, Arena arena, Addr addr); +extern Bool LDIsStaleAny(mps_ld_t ld, Arena arena); extern Bool LDIsStale(mps_ld_t ld, Arena arena, Addr addr); extern void LDAge(Arena arena, RefSet moved); extern void LDMerge(mps_ld_t ld, Arena arena, mps_ld_t from); diff --git a/mps/code/mps.h b/mps/code/mps.h index 5bc46d4d1c9..dfcb894b8c3 100644 --- a/mps/code/mps.h +++ b/mps/code/mps.h @@ -661,6 +661,7 @@ extern void mps_ld_reset(mps_ld_t, mps_arena_t); extern void mps_ld_add(mps_ld_t, mps_arena_t, mps_addr_t); extern void mps_ld_merge(mps_ld_t, mps_arena_t, mps_ld_t); extern mps_bool_t mps_ld_isstale(mps_ld_t, mps_arena_t, mps_addr_t); +extern mps_bool_t mps_ld_isstale_any(mps_ld_t, mps_arena_t); extern mps_word_t mps_collections(mps_arena_t); diff --git a/mps/code/mpsi.c b/mps/code/mpsi.c index dede080ea45..8aa7876a939 100644 --- a/mps/code/mpsi.c +++ b/mps/code/mpsi.c @@ -1480,6 +1480,15 @@ mps_bool_t mps_ld_isstale(mps_ld_t ld, mps_arena_t arena, return (mps_bool_t)b; } +mps_bool_t mps_ld_isstale_any(mps_ld_t ld, mps_arena_t arena) +{ + Bool b; + + b = LDIsStaleAny(ld, arena); + + return (mps_bool_t)b; +} + mps_res_t mps_fix(mps_ss_t mps_ss, mps_addr_t *ref_io) { mps_res_t res; diff --git a/mps/code/mpsicv.c b/mps/code/mpsicv.c index 4a3c95abe83..142498a4502 100644 --- a/mps/code/mpsicv.c +++ b/mps/code/mpsicv.c @@ -468,6 +468,7 @@ static void *test(void *arg, size_t s) mps_ld_add(&ld, arena, obj); if (mps_ld_isstale(&ld, arena, obj)) { + cdie(mps_ld_isstale_any(&ld, arena), "mps_ld_isstale_any"); mps_ld_reset(&ld, arena); mps_ld_add(&ld, arena, obj); } diff --git a/mps/manual/source/glossary/l.rst b/mps/manual/source/glossary/l.rst index 16bf68d24d9..5a143e55f69 100644 --- a/mps/manual/source/glossary/l.rst +++ b/mps/manual/source/glossary/l.rst @@ -239,8 +239,8 @@ Memory Management Glossary: L :term:`references` (and not merely on the identity of the :term:`block` to which the reference refers), and provides a function - (:c:func:`mps_ld_isstale`) to find out whether any of - these references have been changed because a block has + (:c:func:`mps_ld_isstale`) to find out whether a + reference might have been changed because a block has been :term:`moved `. See :ref:`topic-location`. diff --git a/mps/manual/source/guide/advanced.rst b/mps/manual/source/guide/advanced.rst index 54680dcb484..04714d3b7d7 100644 --- a/mps/manual/source/guide/advanced.rst +++ b/mps/manual/source/guide/advanced.rst @@ -335,7 +335,7 @@ any of its keys. If you look up a key in an address-based hash table and fail to find it there, that might be because the table's dependency on the location of the key is stale: that is, if the garbage collector moved the key. The -function :c:func:`mps_ld_isstale` tells you if any of the blocks whose +function :c:func:`mps_ld_isstale` tells you if a block whose locations you depended upon since the last call to :c:func:`mps_ld_reset` might have moved. @@ -354,25 +354,17 @@ locations you depended upon since the last call to return NULL; } -It's important to test :c:func:`mps_ld_isstale` only in case of failure. -The function tells you whether *any* of the dependencies is stale, not -whether a particular dependency is stale. So if ``key`` has not moved, -but some other keys have moved, then if you tested -:c:func:`mps_ld_isstale` first, it would return true and so you'd end up -unnecessarily rehashing the whole table. (It's crucial, however, to -actually test that ``key`` appears in the table, not just that some key -with the same hash does.) +It's important to test :c:func:`mps_ld_isstale` only in case of +failure. The function may report a false positive (returning true +despite the block not having moved). So if ``key`` has not moved, then +if you tested :c:func:`mps_ld_isstale` first, it might return true and +so you'd end up unnecessarily rehashing the whole table. (It's +crucial, however, to actually test that ``key`` appears in the table, +not just that some key with the same hash does.) When a table is rehashed, call :c:func:`mps_ld_reset` to clear the location dependency, and then :c:func:`mps_ld_add` for each key before it is added back to the table. -.. note:: - - Somewhat misleadingly, :c:func:`mps_ld_isstale` takes an address as - its third argument. This address is not tested for staleness: it - appears in the :term:`telemetry stream`, however, where it might be - useful for debugging. - .. note:: After :c:func:`mps_ld_isstale` has returned true, and after diff --git a/mps/manual/source/topic/location.rst b/mps/manual/source/topic/location.rst index 5cf4507264d..d70d689415b 100644 --- a/mps/manual/source/topic/location.rst +++ b/mps/manual/source/topic/location.rst @@ -124,7 +124,7 @@ done in the function that hashes an address: Testing dependencies for staleness ---------------------------------- -When the locations of blocks are used, first carry out the computation +When the location of a block is used, first carry out the computation in the normal way. For example, when looking up a key in an address-based hash table, start by hashing the pointer and looking up the corresponding index in the table. @@ -135,26 +135,26 @@ required: the operation can proceed as usual. But if the operation fails, you might be in one of two cases: -1. the location of these blocks has not been depended on before (for +1. the location of the block has not been depended on before (for example, the key has never been added to the hash table); -2. the location of these blocks has been depended on before (for - example, the key was added to the hash table), but one or more of - the blocks has moved and the dependency has become stale. +2. the location of the block has been depended on before (for example, + the key was added to the hash table), but the block has moved and + the dependency has become stale. At this point you should call :c:func:`mps_ld_isstale`. If it returns -false, then you know that no blocks have moved, so you must be in case +false, then you know that the block has not moved, so you must be in case (1). But if :c:func:`mps_ld_isstale` returns true, you could still be in either case (1) or case (2). All :c:func:`mps_ld_isstale` tells you is -that *some* blocks that have been depended on *might* have moved, not -whether a *particular* block *has* moved. At this point you must: +that the block *might* have moved, not whether the block *has* moved. +At this point you must: 1. reset the location dependency; -2. repeat the computation in some way that doesn't depend on the - old locations of the blocks; and +2. repeat the computation in some way that doesn't depend on the old + locations of all the blocks that were added to that dependency; and 3. re-add a dependency on each block. @@ -285,8 +285,9 @@ Location dependency interface .. c:function:: mps_bool_t mps_ld_isstale(mps_ld_t ld, mps_arena_t arena, mps_addr_t addr) - Determine if any of the dependencies in a :term:`location - dependency` are stale with respect to an :term:`arena`. + Determine if a dependency on the location of a block in a + :term:`location dependency` might be stale with respect to an + :term:`arena`. ``ld`` is the location dependency. @@ -294,34 +295,58 @@ Location dependency interface the same arena that was passed to all calls to :c:func:`mps_ld_add` on ``ld``. - ``addr`` is an address that may appear in :term:`telemetry - ` events related to this call (it will *not* be - tested for staleness). + ``addr`` is the address of the block that is to be tested for + staleness. - The location dependency is examined to determine whether any of - the dependencies encapsulated in it have been made stale with - respect to ``arena``. If any of the dependencies encapsulated in - the location dependency are stale (that is, the blocks whose - location has been depended on have been moved by ``arena``) then - :c:func:`mps_ld_isstale` will return true. If there have been no - calls to :c:func:`mps_ld_add` on ``ld`` since the last call to - :c:func:`mps_ld_reset`, then :c:func:`mps_ld_isstale` will return - false. :c:func:`mps_ld_isstale` may return any value in other - circumstances (but will strive to return false if the blocks - encapsulated in the location dependency have not moved). + If there have been no calls to :c:func:`mps_ld_add` on ``ld`` + since the last call to :c:func:`mps_ld_reset`, then return false. + + If the block at ``addr`` was formerly added to the location + dependency ``ld`` and subsequently moved by ``arena``, then return + true. + + Otherwise, :c:func:`mps_ld_isstale` may return either true or + false. (The function strives to return true in the case where + ``addr`` was added to the location dependency and subsquently + moved, and false otherwise, but cannot ensure this.) .. note:: - :c:func:`mps_ld_isstale` may report a false positive - (returning true despite none of the added addresses having - being moved by the arena) but never a false negative - (returning false when an added address has been moved). + :c:func:`mps_ld_isstale` may report a false positive: it may + return true in the case where ``addr`` was not added to the + location dependency, or in the case where it was added but not + moved. It never reports a false negative. :c:func:`mps_ld_isstale` is thread-safe with respect to itself and with respect to :c:func:`mps_ld_add`, but not with respect to :c:func:`mps_ld_reset`. +.. c:function:: mps_bool_t mps_ld_isstale_any(mps_ld_t ld, mps_arena_t arena) + + Determine if a dependency on the location of *any* block in a + :term:`location dependency` might be stale with respect to an + :term:`arena`. + + ``ld`` is the location dependency. + + ``arena`` is the arena to test for staleness against. It must be + the same arena that was passed to all calls to + :c:func:`mps_ld_add` on ``ld``. + + If there have been no calls to :c:func:`mps_ld_add` on ``ld`` + since the last call to :c:func:`mps_ld_reset`, then return false. + + If any block added to the location dependency ``ld`` has been + moved by ``arena``, then return true. + + Otherwise, :c:func:`mps_ld_isstale_any` may return either true or + false. (The function strives to return true in the case where a + block was added to the location dependency and subsquently moved, + and false otherwise, but cannot ensure this.) + + + .. c:function:: void mps_ld_merge(mps_ld_t dest_ld, mps_arena_t arena, mps_ld_t src_ld) Merge one :term:`location dependency` into another.