diff --git a/mps/code/check.h b/mps/code/check.h index a2450bebd9a..2a08160040e 100644 --- a/mps/code/check.h +++ b/mps/code/check.h @@ -39,6 +39,15 @@ #include "mpslib.h" +/* FIXME: to config.h? */ + +#if defined(MPS_BUILD_LL) || defined(MPS_BUILD_GC) +#define UNEXPECTED(expr) __builtin_expect((expr) != 0, TRUE) +#else +#define UNEXPECTED(expr) (expr) +#endif + + /* ASSERT -- basic assertion * * The ASSERT macro is equivalent to the ISO C assert() except that it is @@ -51,7 +60,7 @@ #define ASSERT(cond, condstring) \ BEGIN \ - if (cond) NOOP; else \ + if (UNEXPECTED(cond)) NOOP; else \ mps_lib_assert_fail(MPS_FILE, __LINE__, (condstring)); \ END diff --git a/mps/code/global.c b/mps/code/global.c index d6522cc30e3..ce8885daf04 100644 --- a/mps/code/global.c +++ b/mps/code/global.c @@ -175,15 +175,13 @@ Bool GlobalsCheck(Globals arenaGlobals) /* This is too expensive to check all the time since we have an expanding shield cache that often has 16K elements instead of 16. */ -#ifdef AVER_AND_CHECK_ALL +#if defined(AVER_AND_CHECK_ALL) { Count depth = 0; for (i = 0; i < arena->shCacheLimit; ++i) { Seg seg = arena->shCache[i]; - if (seg != NULL) { - CHECKD(Seg, seg); - depth += SegDepth(seg); - } + CHECKD(Seg, seg); + depth += SegDepth(seg); } CHECKL(depth <= arena->shDepth); } diff --git a/mps/code/mpmst.h b/mps/code/mpmst.h index ed28ce66214..1ee48063c8d 100644 --- a/mps/code/mpmst.h +++ b/mps/code/mpmst.h @@ -253,7 +253,8 @@ typedef struct SegStruct { /* segment structure */ Tract firstTract; /* first tract of segment */ RingStruct poolRing; /* link in list of segs in pool */ Addr limit; /* limit of segment */ - unsigned depth : ShieldDepthWIDTH; /* see */ + unsigned depth : ShieldDepthWIDTH; /* see design.mps.shield.def.depth */ + BOOLFIELD(cached); /* in shield cache? */ AccessSet pm : AccessLIMIT; /* protection mode, */ AccessSet sm : AccessLIMIT; /* shield mode, */ TraceSet grey : TraceLIMIT; /* traces for which seg is grey */ diff --git a/mps/code/seg.c b/mps/code/seg.c index 2a76f46685d..8f3fe37dcd7 100644 --- a/mps/code/seg.c +++ b/mps/code/seg.c @@ -150,6 +150,7 @@ static Res SegInit(Seg seg, Pool pool, Addr base, Size size, ArgList args) seg->pm = AccessSetEMPTY; seg->sm = AccessSetEMPTY; seg->depth = 0; + seg->cached = FALSE; seg->firstTract = NULL; seg->sig = SegSig; /* set sig now so tract checks will see it */ @@ -220,8 +221,10 @@ static void SegFinish(Seg seg) seg->rankSet = RankSetEMPTY; /* See */ - if (seg->depth > 0) + AVER(seg->depth == 0); + if (seg->cached) ShieldFlush(PoolArena(SegPool(seg))); + AVER(seg->cached == FALSE); limit = SegLimit(seg); @@ -713,17 +716,9 @@ Bool SegCheck(Seg seg) (design.mps.shield.inv.prot.shield). */ CHECKL(BS_DIFF(seg->pm, seg->sm) == 0); - /* An exposed segment is not protected - (design.mps.shield.inv.expose.prot). FIXME: Discovered to be - FALSE when raising the read barrier on a write-protected - segment. RB 2016-03-19 */ - /* CHECKL(seg->depth == 0 || seg->pm == 0); */ - /* FIXME: Wouldn't it be better to have a flag for "in cache" so - that depth > 0 => exposed? */ - - /* All unsynced segments have positive depth + /* All unsynced segments have positive depth or are in the cache (design.mps.shield.inv.unsynced.depth). */ - CHECKL(seg->sm == seg->pm || seg->depth > 0); + CHECKL(seg->sm == seg->pm || seg->depth > 0 || seg->cached); CHECKL(RankSetCheck(seg->rankSet)); if (seg->rankSet == RankSetEMPTY) { diff --git a/mps/code/shield.c b/mps/code/shield.c index 311984e9a21..af1ad934d67 100644 --- a/mps/code/shield.c +++ b/mps/code/shield.c @@ -25,6 +25,12 @@ static Bool shieldSegIsSynced(Seg seg) return SegSM(seg) == SegPM(seg); } +static Bool SegIsExposed(Seg seg) +{ + AVER_CRITICAL(TESTT(Seg, seg)); + return seg->depth > 0; +} + /* shieldSync -- synchronize a segment's protection * @@ -77,6 +83,7 @@ void (ShieldResume)(Arena arena) AVER(arena->insideShield); AVER(arena->suspended); /* It is only correct to actually resume the mutator here if shDepth is 0 */ + /* TODO: Consider actually doing that. */ } @@ -89,34 +96,35 @@ static void protLower(Arena arena, Seg seg, AccessSet mode) AVER_CRITICAL(TESTT(Seg, seg)); AVERT_CRITICAL(AccessSet, mode); - if (SegPM(seg) & mode) { - SegSetPM(seg, SegPM(seg) & ~mode); + if (BS_INTER(SegPM(seg), mode) != 0) { + SegSetPM(seg, BS_DIFF(SegPM(seg), mode)); ProtSet(SegBase(seg), SegLimit(seg), SegPM(seg)); } } -static void shieldFlushEntry(Arena arena, Size i) +/* shieldFlushEntry -- flush a single entry from the cache */ + +static void shieldFlushEntry(Arena arena, Index i) { Seg seg; - AVERT(Arena, arena); - AVER(i < arena->shCacheLimit); + AVERT(Arena, arena); seg = arena->shCache[i]; AVERT(Seg, seg); - AVER(arena->shDepth > 0); - AVER(SegDepth(seg) > 0); - --arena->shDepth; - SegSetDepth(seg, SegDepth(seg) - 1); + AVER(i < arena->shCacheLimit); + AVER(seg->cached); - if (SegDepth(seg) == 0) + if (!SegIsExposed(seg)) shieldSync(arena, seg); - arena->shCache[i] = NULL; /* just to make sure it gets overwritten */ + arena->shCache[i] = NULL; /* just to make sure it can't be reused */ } +/* shieldCacheEntryCompare -- comparison for cache sorting */ + static Compare shieldCacheEntryCompare(void *left, void *right, void *closure) { Seg segA = left, segB = right; @@ -162,32 +170,25 @@ static void shieldFlushEntries(Arena arena) { Addr base = NULL, limit = NULL; AccessSet mode; - Seg seg; - Size i; + Index i; - if (arena->shDepth == 0) { - AVER(arena->shCacheLimit == 0); + if (arena->shCacheLength == 0) { + AVER(arena->shCache == NULL); return; } - AVER(arena->shCache != NULL); - AVER(arena->shCacheLength > 0); - QuickSort((void *)arena->shCache, arena->shCacheLimit, shieldCacheEntryCompare, UNUSED_POINTER); mode = AccessSetEMPTY; for (i = 0; i < arena->shCacheLimit; ++i) { - AVER(arena->shDepth > 0); - - seg = arena->shCache[i]; - arena->shCache[i] = NULL; /* ensure it can't be reused */ + Seg seg = arena->shCache[i]; AVERT(Seg, seg); - AVER(SegDepth(seg) > 0); - --arena->shDepth; - SegSetDepth(seg, SegDepth(seg) - 1); + AVER(seg->cached); + seg->cached = FALSE; + arena->shCache[i] = NULL; /* ensure it can't be reused */ if (!shieldSegIsSynced(seg)) { AVER(SegSM(seg) != AccessSetEMPTY); /* can't match first iter */ @@ -216,8 +217,8 @@ static void shieldFlushEntries(Arena arena) /* shieldCache -- consider adding a segment to the cache * - * If the segment is out of sync, either sync it, or ensure depth > 0, - * and the arena is suspended. + * If the segment is out of sync, either sync it, or ensure it is + * cached and the arena is suspended. */ static void shieldCache(Arena arena, Seg seg) @@ -227,10 +228,10 @@ static void shieldCache(Arena arena, Seg seg) /* Can't fully check seg while we're enforcing its invariants. */ AVER_CRITICAL(TESTT(Seg, seg)); - if (shieldSegIsSynced(seg)) + if (shieldSegIsSynced(seg) || seg->cached) return; - - if (SegDepth(seg) > 0) { /* already in the cache or exposed? */ + + if (SegIsExposed(seg)) { /* This can occur if the mutator isn't suspended, we expose a segment, then raise the shield on it. In this case, the mutator isn't allowed to see the segment, but we don't need to @@ -279,11 +280,6 @@ static void shieldCache(Arena arena, Seg seg) return; } - SegSetDepth(seg, SegDepth(seg) + 1); - AVER_CRITICAL(SegDepth(seg) > 0); /* overflow */ - ++arena->shDepth; - AVER_CRITICAL(arena->shDepth > 0); /* overflow */ - AVER_CRITICAL(arena->shCacheLimit <= arena->shCacheLength); AVER_CRITICAL(arena->shCacheI <= arena->shCacheLimit); @@ -304,13 +300,14 @@ static void shieldCache(Arena arena, Seg seg) arena->shCache[arena->shCacheI] = seg; ++arena->shCacheI; + seg->cached = TRUE; if (arena->shCacheI >= arena->shCacheLimit) arena->shCacheLimit = arena->shCacheI; } -void (ShieldRaise) (Arena arena, Seg seg, AccessSet mode) +void (ShieldRaise)(Arena arena, Seg seg, AccessSet mode) { /* .seg.broken: Seg's shield invariants may not be true at */ /* this point (this function is called to enforce them) so we */ @@ -327,6 +324,7 @@ void (ShieldRaise) (Arena arena, Seg seg, AccessSet mode) /* ensure .inv.unsynced.suspended and .inv.unsynced.depth */ shieldCache(arena, seg); + /* Check cache and segment consistency. */ AVERT(Arena, arena); AVERT(Seg, seg); } @@ -334,13 +332,17 @@ void (ShieldRaise) (Arena arena, Seg seg, AccessSet mode) void (ShieldLower)(Arena arena, Seg seg, AccessSet mode) { - /* Don't check seg or arena, see .seg.broken */ + AVERT(Arena, arena); + AVER(TESTT(Seg, seg)); AVERT(AccessSet, mode); - AVER((SegSM(seg) & mode) == mode); + AVER(BS_INTER(SegSM(seg), mode) == mode); + /* synced(seg) is not changed by the following preserving inv.unsynced.suspended Also inv.prot.shield preserved */ - SegSetSM(seg, SegSM(seg) & ~mode); + SegSetSM(seg, BS_DIFF(SegSM(seg), mode)); protLower(arena, seg, mode); + + /* Check cache and segment consistency. */ AVERT(Arena, arena); AVERT(Seg, seg); } @@ -352,8 +354,6 @@ void (ShieldEnter)(Arena arena) AVER(!arena->insideShield); AVER(arena->shDepth == 0); AVER(!arena->suspended); - AVER(arena->shCacheLimit <= arena->shCacheLength); - AVER(arena->shCacheI <= arena->shCacheLimit); shieldCacheReset(arena); @@ -386,6 +386,7 @@ void (ShieldLeave)(Arena arena) AVER(arena->insideShield); ShieldFlush(arena); + /* Cache is empty so .inv.outside.depth holds */ AVER(arena->shDepth == 0); @@ -396,6 +397,20 @@ void (ShieldLeave)(Arena arena) arena->suspended = FALSE; } arena->insideShield = FALSE; + +#ifdef SHIELD_DEBUG + { + Seg seg; + if (SegFirst(&seg, arena)) + do { + AVER(!seg->cached); + AVER(shieldSegIsSynced(seg)); + /* You can directly set protections here to see if it makes a + difference. */ + ProtSet(SegBase(seg), SegLimit(seg), SegPM(seg)); + } while(SegNext(&seg, arena, seg)); + } +#endif } @@ -434,31 +449,33 @@ void (ShieldExpose)(Arena arena, Seg seg) AVER_CRITICAL(arena->insideShield); SegSetDepth(seg, SegDepth(seg) + 1); + AVER_CRITICAL(SegDepth(seg) > 0); /* overflow */ ++arena->shDepth; - /* */ - AVER_CRITICAL(arena->shDepth > 0); - AVER_CRITICAL(SegDepth(seg) > 0); - if (SegPM(seg) & mode) + AVER_CRITICAL(arena->shDepth > 0); /* overflow */ + + if (BS_INTER(SegPM(seg), mode)) ShieldSuspend(arena); - /* This ensures inv.expose.prot */ + /* Ensure design.mps.shield.inv.expose.prot. */ protLower(arena, seg, mode); } +/* ShieldCover -- declare MPS no longer needs access to seg */ + void (ShieldCover)(Arena arena, Seg seg) { /* */ AVERT_CRITICAL(Arena, arena); AVERT_CRITICAL(Seg, seg); AVER_CRITICAL(SegPM(seg) == AccessSetEMPTY); - - AVER_CRITICAL(arena->shDepth > 0); + AVER_CRITICAL(SegDepth(seg) > 0); SegSetDepth(seg, SegDepth(seg) - 1); + AVER_CRITICAL(arena->shDepth > 0); --arena->shDepth; - /* ensure inv.unsynced.depth */ + /* Ensure design.mps.shield.inv.unsynced.depth. */ shieldCache(arena, seg); } diff --git a/mps/design/shield.txt b/mps/design/shield.txt index 09e57d43e18..c3369750789 100644 --- a/mps/design/shield.txt +++ b/mps/design/shield.txt @@ -115,13 +115,9 @@ segment should be unprotected, and the Shield could re-instate hardware protection. However, as a performance-improving hysteresis, the Shield defers -re-protection, maintaining a cache reasons that a segment no longer -had a reason to be collector-accessible. Presence in the cache counts -as a reason: segments in the cache have ``seg->depth`` increased by -one. As segments get pushed out of the cache, or at ``ShieldLeave()``, -this artificial reason is decremented from ``seg->depth``, and (if -``seg->depth`` is now zero) the deferred reinstatement of hardware -protection happens. +re-protection, maintaining a cache of segments that no longer have a +reason to be collector-accessible. While a segment is in the cache, +it has ``seg->cached`` set to TRUE. This hysteresis allows the MPS to proceed with garbage collection during a pause without actually setting hardware protection until it @@ -154,20 +150,22 @@ same, and unsynced otherwise. .def.depth: The depth of a segment is defined as: - | depth ≔ #exposes − #covers + #(in cache), where + | depth ≔ #exposes − #covers, where | #exposes = the total number of times the seg has been exposed | #covers = the total number of times the seg has been covered - | #(in cache) = the number of times the seg appears in the cache -The cache is initially empty and Cover should not be called without a -matching Expose, so this figure should always be non-negative. +The cache is initially empty and ``ShieldCover`` should not be called +without a matching ``ShieldExpose``, so this figure should always be +non-negative. -.def.total.depth: The total depth is the sum of the depth over -all segments. +.def.total.depth: The total depth is the sum of the depth over all +segments. -.def.outside: Being outside the shield is being between calls to leave -and enter, and similarly .def.inside: being inside the shield is being -between calls to enter and leave. +.def.outside: Being outside the shield is being between calls to +``ShieldLeave`` and ``ShieldEnter``, and similarly .def.inside: being +inside the shield is being between calls to ``ShieldEnter`` and +``ShieldLeave``. [In a multi-threaded MPS this would be per-thread. +RB 2016-03-18] .def.suspended: Suspended is true iff the mutator is suspended. @@ -196,13 +194,15 @@ shield. .inv.unsynced.suspended: If any segment is not synced, the mutator is suspended. -.inv.unsynced.depth: All unsynced segments have positive depth. +.inv.unsynced.depth: All unsynced segments have positive depth or are +in the cache. .inv.outside.depth: The total depth is zero while outside the shield. .inv.prot.shield: The prot mode is never more than the shield mode. -.inv.expose.prot: An exposed seg is not protected. +.inv.expose.prot: An exposed seg is not protected in the mode it was +exposed with. Proof Hints @@ -236,6 +236,17 @@ _`.ideas`: There never was an initial design document, but [RB_1995-11-29]_ and [RB_1995-11-30]_ contain some initial ideas. +Improvement Ideas +----------------- + +.improv.mass-expose: If protection calls have a high overhead it might +be good to pre-emptively unprotect large ranges of memory when we +expose one segment. With the current design this would mean +discovering adjacent shielded segments and adding them to the cache. +The collector should take advantage of this by preferentially scanning +exposed segments during a pause. + + References ----------