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

@@ -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 <http://www.ravenbrook.com/>.
+ * Copyright (C) 2001-2013 Ravenbrook Limited <http://www.ravenbrook.com/>.
  * All rights reserved.  This is an open source license.  Contact
  * Ravenbrook for commercial licensing options.
  * 

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);

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);
 

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;

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);
   }

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 <moving garbage collector>`. See
             :ref:`topic-location`.
 

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

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
-    <telemetry stream>` 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.