From e034e7aeea4c8f0c0bfc249d430de3eb814e12fd Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Mon, 14 Mar 2016 20:10:14 +0000 Subject: [PATCH] Address points made by rb in review Copied from Perforce Change: 190031 ServerID: perforce.ravenbrook.com --- mps/code/arena.c | 19 ++++++++++--------- mps/code/eventdef.h | 2 +- mps/code/global.c | 4 ++-- mps/code/mpm.h | 4 ++-- mps/code/mpmst.h | 2 +- mps/code/policy.c | 3 +++ mps/code/trace.c | 33 ++++++++++++++++++--------------- mps/code/traceanc.c | 2 +- mps/design/type.txt | 6 ++++-- 9 files changed, 42 insertions(+), 33 deletions(-) diff --git a/mps/code/arena.c b/mps/code/arena.c index 17634daecbd..dc7ae37b15c 100644 --- a/mps/code/arena.c +++ b/mps/code/arena.c @@ -1320,15 +1320,6 @@ Size ArenaAvail(Arena arena) /* ArenaCollectable -- return estimate of collectable memory in arena */ Size ArenaCollectable(Arena arena) -{ - /* Conservative estimate -- see job003929. */ - return ArenaScannable(arena); -} - - -/* ArenaScannable -- return estimate of scannable memory in arena */ - -Size ArenaScannable(Arena arena) { /* Conservative estimate -- see job003929. */ Size committed = ArenaCommitted(arena); @@ -1338,6 +1329,16 @@ Size ArenaScannable(Arena arena) } +/* ArenaAccumulateTime -- accumulate time spent tracing */ + +void ArenaAccumulateTime(Arena arena, Clock start, Clock end) +{ + AVERT(Arena, arena); + AVER(start <= end); + arena->tracedTime += (end - start) / (double) ClocksPerSec(); +} + + /* ArenaExtend -- Add a new chunk in the arena */ Res ArenaExtend(Arena arena, Addr base, Size size) diff --git a/mps/code/eventdef.h b/mps/code/eventdef.h index cb7b590274c..312f257ee0f 100644 --- a/mps/code/eventdef.h +++ b/mps/code/eventdef.h @@ -674,7 +674,7 @@ PARAM(X, 4, W, notCondemned) /* collectible but not condemned bytes */ \ PARAM(X, 5, W, foundation) /* foundation size */ \ PARAM(X, 6, W, white) /* white reference set */ \ - PARAM(X, 7, W, quantumWork) /* work constituting a quantum */ + PARAM(X, 7, W, quantumWork) /* tracing work to be done in each poll */ #define EVENT_VMCompact_PARAMS(PARAM, X) \ PARAM(X, 0, W, vmem0) /* pre-collection reserved size */ \ diff --git a/mps/code/global.c b/mps/code/global.c index fae5b0bb2dd..39e83714f82 100644 --- a/mps/code/global.c +++ b/mps/code/global.c @@ -739,7 +739,7 @@ void (ArenaPoll)(Globals globals) /* Don't count time spent checking for work, if there was no work to do. */ if (workWasDone) { - ArenaAccumulateTime(arena, start); + ArenaAccumulateTime(arena, start, ClockNow()); } AVER(!PolicyPoll(arena)); @@ -801,7 +801,7 @@ Bool ArenaStep(Globals globals, double interval, double multiplier) } while (now < intervalEnd); if (workWasDone) { - ArenaAccumulateTime(arena, start); + ArenaAccumulateTime(arena, start, now); } return workWasDone; diff --git a/mps/code/mpm.h b/mps/code/mpm.h index 2cd17c27135..6cbc676ab00 100644 --- a/mps/code/mpm.h +++ b/mps/code/mpm.h @@ -397,6 +397,7 @@ extern Res TraceCreate(Trace *traceReturn, Arena arena, int why); extern void TraceDestroyInit(Trace trace); extern void TraceDestroyFinished(Trace trace); +extern Bool TraceIsEmpty(Trace trace); extern Res TraceAddWhite(Trace trace, Seg seg); extern Res TraceCondemnZones(Trace trace, ZoneSet condemnedSet); extern Res TraceStart(Trace trace, double mortality, double finishingTime); @@ -537,7 +538,6 @@ extern Bool ArenaGrainSizeCheck(Size size); #define AddrIsArenaGrain(addr, arena) AddrIsAligned(addr, ArenaGrainSize(arena)) #define SizeArenaGrains(size, arena) SizeAlignUp(size, ArenaGrainSize(arena)) #define SizeIsArenaGrains(size, arena) SizeIsAligned(size, ArenaGrainSize(arena)) -#define ArenaAccumulateTime(arena, start) ((arena)->tracedTime += (ClockNow() - (start)) / (double) ClocksPerSec()) extern void ArenaEnterLock(Arena arena, Bool recursive); extern void ArenaLeaveLock(Arena arena, Bool recursive); @@ -572,6 +572,7 @@ extern Bool ArenaHasAddr(Arena arena, Addr addr); extern Res ArenaAddrObject(Addr *pReturn, Arena arena, Addr addr); extern void ArenaChunkInsert(Arena arena, Chunk chunk); extern void ArenaChunkRemoved(Arena arena, Chunk chunk); +extern void ArenaAccumulateTime(Arena arena, Clock start, Clock now); extern void ArenaSetEmergency(Arena arena, Bool emergency); extern Bool ArenaEmergency(Arena arean); @@ -628,7 +629,6 @@ extern Res ArenaNoGrow(Arena arena, LocusPref pref, Size size); extern Size ArenaAvail(Arena arena); extern Size ArenaCollectable(Arena arena); -extern Size ArenaScannable(Arena arena); extern Res ArenaExtend(Arena, Addr base, Size size); diff --git a/mps/code/mpmst.h b/mps/code/mpmst.h index b7267e2cba3..4e848f5da57 100644 --- a/mps/code/mpmst.h +++ b/mps/code/mpmst.h @@ -486,7 +486,7 @@ typedef struct TraceStruct { Size condemned; /* condemned bytes */ Size notCondemned; /* collectable but not condemned */ Size foundation; /* initial grey set size */ - Work quantumWork; /* collection work constituting a quantum */ + Work quantumWork; /* tracing work to be done in each poll */ STATISTIC_DECL(Count greySegCount); /* number of grey segs */ STATISTIC_DECL(Count greySegMax); /* max number of grey segs */ STATISTIC_DECL(Count rootScanCount); /* number of roots scanned */ diff --git a/mps/code/policy.c b/mps/code/policy.c index 82709f7376f..a9bc710be1b 100644 --- a/mps/code/policy.c +++ b/mps/code/policy.c @@ -325,6 +325,8 @@ Bool PolicyStartTrace(Trace *traceReturn, Arena arena) res = policyCondemnChain(&mortality, firstChain, trace); if (res != ResOK) /* should try some other trace, really @@@@ */ goto failCondemn; + if (TraceIsEmpty(trace)) + goto nothingCondemned; trace->chain = firstChain; ChainStartGC(firstChain, trace); res = TraceStart(trace, mortality, trace->condemned * TraceWorkFactor); @@ -336,6 +338,7 @@ Bool PolicyStartTrace(Trace *traceReturn, Arena arena) } /* (dynamicDeferral > 0.0) */ return FALSE; +nothingCondemned: failCondemn: TraceDestroyInit(trace); failStart: diff --git a/mps/code/trace.c b/mps/code/trace.c index 6973d8ce63d..6d9cb0f0f98 100644 --- a/mps/code/trace.c +++ b/mps/code/trace.c @@ -339,6 +339,15 @@ static ZoneSet traceSetWhiteUnion(TraceSet ts, Arena arena) } +/* TraceIsEmpty -- return TRUE if trace has no condemned segments */ + +Bool TraceIsEmpty(Trace trace) +{ + AVERT(Trace, trace); + return trace->condemned == 0; +} + + /* TraceAddWhite -- add a segment to the white set of a trace */ Res TraceAddWhite(Trace trace, Seg seg) @@ -391,7 +400,6 @@ Res TraceCondemnZones(Trace trace, ZoneSet condemnedSet) Seg seg; Arena arena; Res res; - Bool haveWhiteSegs = FALSE; AVERT(Trace, trace); AVER(condemnedSet != ZoneSetEMPTY); @@ -418,14 +426,10 @@ Res TraceCondemnZones(Trace trace, ZoneSet condemnedSet) res = TraceAddWhite(trace, seg); if(res != ResOK) goto failBegin; - haveWhiteSegs = TRUE; } } while (SegNext(&seg, arena, seg)); } - if (!haveWhiteSegs) - return ResFAIL; - EVENT3(TraceCondemnZones, trace, condemnedSet, trace->white); /* The trace's white set must be a subset of the condemned set */ @@ -434,7 +438,7 @@ Res TraceCondemnZones(Trace trace, ZoneSet condemnedSet) return ResOK; failBegin: - AVER(!haveWhiteSegs); /* See .whiten.fail. */ + AVER(TraceIsEmpty(trace)); /* See .whiten.fail. */ return res; } @@ -697,7 +701,7 @@ found: trace->condemned = (Size)0; /* nothing condemned yet */ trace->notCondemned = (Size)0; trace->foundation = (Size)0; /* nothing grey yet */ - trace->quantumWork = (Work)0; /* no work done yet */ + trace->quantumWork = (Work)0; /* computed in TraceStart */ STATISTIC(trace->greySegCount = (Count)0); STATISTIC(trace->greySegMax = (Count)0); STATISTIC(trace->rootScanCount = (Count)0); @@ -1482,7 +1486,6 @@ static Res traceCondemnAll(Trace trace) Res res; Arena arena; Ring poolNode, nextPoolNode, chainNode, nextChainNode; - Bool haveWhiteSegs = FALSE; arena = trace->arena; AVERT(Arena, arena); @@ -1501,12 +1504,11 @@ static Res traceCondemnAll(Trace trace) res = TraceAddWhite(trace, seg); if (res != ResOK) goto failBegin; - haveWhiteSegs = TRUE; } } } - if (!haveWhiteSegs) + if (TraceIsEmpty(trace)) return ResFAIL; /* Notify all the chains. */ @@ -1525,7 +1527,7 @@ failBegin: * pool class that fails to whiten a segment, then this assertion * will be triggered. In that case, we'll have to recover here by * blackening the segments again. */ - AVER(!haveWhiteSegs); + AVER(TraceIsEmpty(trace)); return res; } @@ -1645,8 +1647,8 @@ Res TraceStart(Trace trace, double mortality, double finishingTime) /* integer, so try to make sure it fits. */ if(nPolls >= (double)LONG_MAX) nPolls = (double)LONG_MAX; - /* One quantum of work equals scanning work divided by number of - * polls, plus one to ensure it's not zero. */ + /* One quantum of work equals total tracing work divided by number + * of polls, plus one to ensure it's not zero. */ trace->quantumWork = (trace->foundation + sSurvivors) / (unsigned long)nPolls + 1; } @@ -1671,9 +1673,10 @@ Res TraceStart(Trace trace, double mortality, double finishingTime) } -/* traceWork -- a measure of the work done for this trace +/* traceWork -- a measure of the work done for this trace. * - * .work: Segment and root scanning work is the measure. */ + * See design.mps.type.work. + */ #define traceWork(trace) ((Work)((trace)->segScanSize + (trace)->rootScanSize)) diff --git a/mps/code/traceanc.c b/mps/code/traceanc.c index e60f8ffcdb3..4730c7a0f54 100644 --- a/mps/code/traceanc.c +++ b/mps/code/traceanc.c @@ -590,7 +590,7 @@ void ArenaPark(Globals globals) TRACE_SET_ITER_END(ti, trace, arena->busyTraces, arena); } - ArenaAccumulateTime(arena, start); + ArenaAccumulateTime(arena, start, ClockNow()); /* All traces have finished so there must not be an emergency. */ AVER(!ArenaEmergency(arena)); diff --git a/mps/design/type.txt b/mps/design/type.txt index dc3c3b8cf99..5f546e1e32e 100644 --- a/mps/design/type.txt +++ b/mps/design/type.txt @@ -669,8 +669,10 @@ _`.word.ops`: ``WordIsAligned()``, ``WordAlignUp()``, _`.work`: ``Work`` is an unsigned integral type representing accumulated work done by the collector. -_`.work.impl`: Work is currently implemented as a count of bytes -scanned by the collector. +_`.work.impl`: Work is implemented as a count of the bytes scanned by +the collector in segments and roots. This is a very crude measure, +because it depends on the scanning functions supplied by the mutator, +which we know very little about. ``typedef Word ZoneSet``