| Title | mps_arena_step unclamps the arena |
| Status | closed |
| Priority | optional |
| Assigned user | Gareth Rees |
| Organization | Ravenbrook |
| Description | If you call mps_arena_step, the arena is unclamped (unless there was nothing to do). This means that if the arena was in the parked state or the clamped state before the call, it changes to the unclamped state. Evidence: mps_arena_step in mpsi.c [1] calls ArenaStep in globals.c [2] which calls ArenaStartCollect in traceanc.c [3] which calls ArenaRelease on all paths through the code. This means that mps_arena_step is not suitable for the use case described in [4] whereby a client program clamps the arena and then calls mps_arena_step when it is idle. |
| Analysis | Change the behaviour of mps_arena_step so that: 1. If the arena was in the parked state state before the call to mps_arena_step, change to the clamped state (not the unclamped state). 2. If the arena was in the clamped state state before the call to mps_arena_step, leave it in the clamped state. GDR: the above describes the behaviour that I have documented. GDR 2013-03-08: In ArenaStep the code that unclamps the arena is as follows: if (arenaShouldCollectWorld(arena, interval, multiplier, start, clocks_per_sec)) { ArenaStartCollect(globals, TraceStartWhyOPPORTUNISM); arena->lastWorldCollect = start; stepped = TRUE; } (This ignores the result code from ArenaStartCollect, which seems wrong, so I fixed that at the same time.) The function ArenaStartCollect just calls ArenaPark, TraceStartCollectAll, and ArenaRelease (plus a bit of error handling). In the case of ArenaStep we can safely omit the call to ArenaPark (because arenaShouldCollectWorld returns false if there is a collection in progress), and we don't want the call to ArenaRelease. So we can just call TraceStartCollectAll. The resulting code is as follows: if (arenaShouldCollectWorld(arena, interval, multiplier, start, clocks_per_sec)) { Res res; Trace trace; res = TraceStartCollectAll(&trace, arena, TraceStartWhyOPPORTUNISM); if (res == ResOK) { arena->lastWorldCollect = start; stepped = TRUE; } } |
| How found | inspection |
| Evidence | [1] <https://info.ravenbrook.com/project/mps/master/code/mpsi.c>[2] < https://info.ravenbrook.com/project/mps/master/code/global.c>[3] < https://info.ravenbrook.com/project/mps/master/code/traceanc.c>[4] < https://info.ravenbrook.com/mail/2003/01/03/14-13-25/0.txt> |
| Observed in | 1.110.0 |
| Created by | Gareth Rees |
| Created on | 2012-10-18 16:17:53 |
| Last modified by | Gareth Rees |
| Last modified on | 2013-03-08 15:27:43 |
| History | 2012-10-18 GDR Created. 2013-03-08 GDR Justification for the fix. |
| Change | Effect | Date | User | Description |
|---|---|---|---|---|
| 181098 | closed | 2013-03-08 15:13:16 | Gareth Rees | Make mps_arena_step suitable for purpose: ArenaStep now calls TraceStartCollectAll directly (not via ArenaStartCollect) so that it no longer unclamps the arena as a side effect. Add test case: steptest now runs with the arena clamped, and checks that mps_arena_step does not unclamp it. |