Make extension callbacks part of the public MPS

[rptb1]

Fixes #212 .

Progress towards resolving #110 .

Part of the plan to meet Configura's requirements by separating and reviewing each implementation.

This branch cherry-picks and edits b928fa2 from #76 to only include the arena extension callback feature.

[rptb1]

We call the extended callback in VMArenaCreate so that the primary chunk is registered

mps/code/arenavm.c

Line 723 in 9919050

vmArena->extended(arena, chunk->base, chunkSize);

but we don't call contracted when chunks are destroyed by VMArenaDestroy That seems inconsistent.

Note that although VMArenaDestroy destroys all chunks

mps/code/arenavm.c

Lines 777 to 780 in 9919050

	/* Destroy all chunks, including the primary. See
	* <design/arena#.chunk.delete> */
	arena->primary = NULL;
	TreeTraverseAndDelete(&arena->chunkTree, vmChunkDestroy, UNUSED_POINTER);

it's not vmChunkDestroy that calls contracted. That's only called by vmChunkCompact.

mps/code/arenavm.c

Lines 1239 to 1240 in 9919050

	(*vmArena->contracted)(arena, base, size);
	vmChunkDestroy(tree, UNUSED_POINTER);

We should consider the effect of changing this behaviour on existing clients.

[rptb1]

So, VMChunkCreate (which should be named vmChunkCreate) is called from two places:

1. VMArenaCreate, where it creates the primary chunk, and
2. VMArenaGrow, where it creates all subsequent chunks.

We explicitly call the extended callback in both cases.

vmChunkDestroy (correctly named) is called from:

1. VMArenaDestroy, where it destroys all chunks via a TreeTraverse, and
2. vmChunkCompact, where we destroy chunks with nothing in them when trying to compact the arena.

We explicitly call the contracted callback only in vmChunkCompact.

It seems likely to me that we can call the extended and contracted callbacks from VMChunkCreate and vmChunkDestroy only, simplifying the code and eliminating the possibility of this inconsistency recurring.

[thejayps]

In the test extcon.c we observed on Rockhopper that the arena would mostly extend 50 times with the current test setup, but sometimes only 34 times. This seems odd and needs further investigation.

[thejayps]

Executing proc.review.entry

1. Start time 15:45
2. proc.review.entry.criteria: It isn't clear how to select appropriate criteria for a review, other than the stipulation that entry.universal should always apply.
   !!!Procedure unclear: @thejayps continues anyway
   I'm choosing to copy the entry criteria used for the recent mps_pool_walk Pull Request since the work is similar. There are also no other documents titled "entry" in the Procedure Directory. So I've chosen to continue by applying entry.universal, entry.design, entry.impl.
3. entry.universal.author: This work was done by git cherry picking and manually isolating specific changes that form the implementation of extension callbacks from Configura's custom production MPS source. The recent work was performed by myself (I give permission) and @rptb1 (attempted to start proc.review.entry on this yesterday, but automated checks were failing. Permission assumed). Configura have given us permission to do this work: https://info.ravenbrook.com/mail/2022/01/18/12-53-38/0/
4. entry.universal.source-available: 2 mail threads: The story behind "Ravenbrook MPS sources are out of sync with
   Configura's sources" and Gathering information on Configura specific functionality describe the approach to this work. (Links to the base email only, p4 grep can find all the subsequent responses).
5. entry.universal.source-approved - the current definition of this tag is unclear.
   !!!Procedure error: @thejayps continues anyway
   I'll respond according to my interpretation: since many components of the source documents are not subject to procedures that have a "review exit", this criteria is difficult to apply. The intention of this criteria could be to guard against wasting time reviewing changes that themselves are based on as yet unreviewed changes which may fail their own review. The mail threads describing our work on this up to this point are not documents that are normally subject to any review process, and as such are UNREVIEWED.
6. entry.design.rfc this PR introduces extension callbacks, I do not know what historic review processes were in places when the original design documents were delivered, however I assume they were in place and took place. In this case, the implementation of callback functions is significantly trivial a concept that lacking reviewed design documentation should not pose a risk to the review. We must also remember that the aim of this work is to synchronise codebases, so the question "Is this the right design approach?" is not currently relevant. I'm choosing to continue.
7. Entry passed
8. End time 16:51
9. Entry took 66 minutes

[thejayps]

Executing proc.review.plan

1. Start time 1428
2. Note that there were changes to the product document subsequent to proc.review.entry, because thinking about planning caused me to decide the changes were of a scale that would be difficult to review. The change has been simplified. The change still passes internal checks and I deem there not to be any other change to the entry criteria.
3. @thejayps and @UNAA008 are expected to be available. @rptb1 might be available.
4. Change is +300 of code and +100 of documentation. Most of the code is a new testbench. The existing functionality that is being tested already exists in the MPS, this change's main purpose is to wire that functionality through to the public api. Estimate 30 mins code change review at 10 loc per min
5. Consider roles:
6. proc.review.role.check.correctness .consistency and .source are most important
7. .source needs to consider keep in mind the somewhat dual purpose of this change: implement a new feature and merge capabilities
8. Roles TBD
9. proc.review.plan.tactics: Using 10 loc/minute rule: Code checking is about 0.5h @ 10 loc/min for code correctness. This may be extended with some contingency depending on the resource allocation.
10. proc.review.plan.source see MPS does not support stack unwinding under Windows when it is managing code #212
11. Planning paused at 1459. Roles still to be assigned

[thejayps]

executing proc.review.kickoff

Link to rules directory for checkers. All rules documents except rule.code.python.rst

[UNAA008]

My review task was to check extcon.c.
I spent a total of 1 hour doing this.
Here are my comments:
m Copyright date in header inconsistent with that in footer.

[Fix: Updated leader comment in 2833d45 .]

q .limitation on not checking numbers of extensions and contractions.
Is this a dangerous omission?

[Answer: Not very dangerous. The test could be altered to use manual allocation and try to correlate allocations and frees with extensions and contractions, as a form of white box testing. Raise #234 .]

m An explanantion of why dylan interfaces are being used would be useful
(line 28 is the first mention)

[Fix: Added explanation in 2833d45 .]

m Not all of the #if 0 exclusions have comments to explain them.
I would prefer to see them marked so that it is obvious when they
can be removed.

[Ignore: duplicate issue already fixed.]

[rptb1]

4 minor defects found during logging.

[thejayps]

Begin logging at 1313

@UNAA008: 30mins + 30mins checking so far, found 3 minor issues and has a question.
@rptb1 5 minor issues, spent 30 mins yesterday checking
@thejayps 1 major issue and 1 minor issue, 30 mins checking yesterday.

@thejayps introduces major issue
.new.major @rptb1 the documentation for contraction also needs to be accurate and clear
@rptb1 talks through minor issues
@UNAA008 talks through minor issues
Logging finished at 1406
Logging took 53 mins

Note: unresolved .new.major defect raised by @rptb1 has been responded to here #213 (comment)

[thejayps]

Begin brainstorming

started at 1425
brainstorm ideas by @rptb1

1. Insist that documentation is produced alongside code, preferably by the same authors, and both are reviewed together.
2. In cases where the above hasn't happened - arrange for the author to check the documentation, and of course both checked against the original problem.
   Action: ensure that rules exist to make these things happen.
   Brainstorming finished at 1439 - 14 mins

[thejayps]

Executing proc.review.edit

Start time 2023/05/23 0031

[thejayps]

M: Documentation doesn't clearly describe the behaviour of the callbacks. extended is called when the arena is created, however a new user may write their own program assuming that the callback is only called once an already existing arena is extended. We need to be clear that extension also means creation.

m: the contracted callback is not called when the arena is destoyed. This could be trivially important in practical use cases but is inconsistent with the above.

Updated documentation in arena.rst to reflect current behaviour.

[thejayps]

Paused proc.review.edit

Commit of changes so far: b8ac0d1

Pause time 2023/05/23 0204

[thejayps]

Executing [proc.review.exit] (https://github.com/Ravenbrook/mps/blob/586336a7021c2276a3f798ad10e6e4ab5f438226/procedure/review.rst)

1. Start time 2102
2. All "conversations" according to git have been resolved.
3. Transgression: exit.universal.defect In the comment Make extension callbacks part of the public MPS #213 (comment) @rptb1 raises a new major defect: "the documentation for contraction also needs to be accurate and clear" - there is no obvious response to this defect being raised in the comment.

Fixed: in fb93d86 where callbacks were redesigned to be more consistent, and documentation was updated.

[thejayps]

4. All questions have been answered
5. Transgression: the brainstorming ideas aren't linked to a defect: Make extension callbacks part of the public MPS #213 (comment). I believe/recall that the suggestions were regarding the single major defect here: Make extension callbacks part of the public MPS #213 (review)

exit.universal.imp.issue : #236

.exit paused at 2140

[thejayps]

Resuming .exit at 1030
5. exit.universal.record Start and end times are mostly recorded, and activity durations calculated in most of these cases. Timings for .edit need better recording. Transgression: no "estimated remaining defects" number is noted. Estimate here now:
6. .estimation: 2 Major defects were found. (one by @thejayps and a new one by @rptb1). Both were related, in fact the same defect applying to 2 opposite areas of code. We could argue that this is 1 defect, but go with 2 for safety. According to .calc.defects-remaining we have 2 options: one calculation uses the number of pages - so we'll disregard this one until we know what a "page" means in github. Instead, use the "75%" method suggested by GavinM which I interpret to mean "We find ~75%" of major defects" -> 2 Major defects = 75% of the total. Total = 2 / 0.75 = 2.7 Major defects in total, so 0.7 (~1) defects remain.
7. .rates: rates are slow and seemed appropriate while staff are trained (this was the first review of its kind). No danger that rates were too quick. Note that "the optimum rates" are not well defined in proc.exit.universal
8. .veto: no vetos (author and leader = @thejayps)
9. Revised change passed (exit.pass)

Total time for exit: 85 mins

[thejayps]

exit.pass at 1316

[thejayps]

Estimates (person mins)
.calc.manpower-used: (note task re gender specific tags)
entry 66
plan 30
ko+check+log+bs (60+30+53+14)3
edit 120 + 1202
exit 85
~= 17 person-hours
.calc.manpower-saved:
2 Major defects *10 person hours = 20 hours

.calc.defects-remaining: 0-1 see calc here

[thejayps]

Executing proc.merge.pull-request from #228

1. Started 1410
2. Checklist passed (no tests were run locally)
3. End time 1420
4. Merge took 10 mins