Source code contains ancient @@@@ to-do markers

[rptb1]

Long ago, before the FIXME convention became popular, the MPS used "@@@@" to do something similar. There are still about 80 of those in the code, but they are low risk and not dealt with by this branch (see purpose above).

Originally posted by @rptb1 in #119 (comment)

[rptb1]

We should examine each of these and determine whether it really is low risk. If so, it can be converted to a TODO. If not, it should be raised as an issue.

This work might be an interesting tour of the MPS for training purposes, or be used to refresh the design documentation.

[rptb1]

Here are the markers at 8635e90 :

./code/seg.c:932:    /* @@@@ What can be checked about the read barrier? */
./code/poolamc.c:316:    /* @@@@ This misses a header-sized pad at the end. */
./code/poolamc.c:869:  /* @@@@ Make sure that segments aren't buffered by forwarding */
./code/poolamc.c:1174:        /* @@@@ We should detach it, but can't for technical */
./code/poolamc.c:1333:  /* @@@@ Shouldn't p be set to BufferLimit here?! */
./code/poolamc.c:1422:      /* @@@@ Are we sure we don't need scan the rest of the */
./code/poolamc.c:1436:  /* <design/poolamc#.seg-scan.finish> @@@@ base? */
./code/dbgpool.c:149:  /* @@@@ Tag parameters should be taken from options, but tags have */
./code/dbgpool.c:162:  /* @@@@ This parses a user argument, options, so it should really */
./code/dbgpool.c:176:  /* @@@@ This parses a user argument, options, so it should really */
./code/dbgpool.c:481:  /* @@@@ shields? */
./code/dbgpool.c:512:  /* @@@@ shields? */
./code/tract.c:209:  /* @@@@ Is BootAllocated always right? */
./code/poolawl.c:30: * (non-barrier) scanning  activity. (@@@@ This is a deficiency in MPS).
./code/poolawl.c:767:    /* pre-flip phase. @@@@ ('coz then we'll be allocating white) */
./code/sac.c:29:    /* @@@@ ignoring shields for now */
./code/sac.c:139:  /* @@@@ It would be better to dynamically adjust the smallest class */
./code/sac.c:296:    /* @@@@ ignoring shields for now */
./code/sac.c:308:  /* @@@@ ignoring shields for now */
./code/sac.c:329:    /* @@@@ ignoring shields for now */
./code/sac.c:371:    /* @@@@ ignoring shields for now */
./code/walk.c:117: * creating a trace without also condemning part of the heap. (@@@@ This
./code/global.c:658:    /* @@@@ The code below assumes that Roots and Segs are disjoint. */
./code/global.c:710: * @@@@ This is where time is "stolen" from the mutator in addition
./code/global.c:715: * @@@@ Perhaps this should be based on a process table rather than a
./code/global.c:1084:  /* @@@@ What about grey rings? */
./code/poolams.c:1307:  /* @@@@ This isn't quite right for multiple traces. */
./code/poolams.c:1352:  /* @@@@ This isn't quite right for multiple traces. */
./code/poolams.c:1441:  /* @@@@ We should check that we're not in the grey mutator phase */
./code/trace.c:57:  /* @@@@ checks for counts missing */
./code/trace.c:190:      /* @@@@ What can be checked here? */
./code/trace.c:196:      /* @@@@ Assert that mutator is grey for trace. */
./code/trace.c:203:      /* @@@@ Assert that mutator is black for trace. */
./code/trace.c:209:      /* @@@@ Assert that grey set is empty for trace. */
./code/trace.c:214:      /* @@@@ Assert that grey and white sets is empty for trace. */
./code/trace.c:224:  /* @@@@ checks for counts missing */
./code/trace.c:646:  /* early, before the pool contents.  @@@@ This isn't correct if there are */
./code/trace.c:673:  /* @@@@ When write barrier collection is implemented, this is where */
./code/trace.c:857: * paths in this file don't work.  @@@@ */
./code/trace.c:1176: * @@@@ During scanning, the segment should be write-shielded to prevent
./code/trace.c:1624:  /* @@@@ Instead of iterating over all the segments, we could */
./code/trace.c:1785:  if(res != ResOK) /* should try some other trace, really @@@@ */
./code/trace.c:1790:    /* Run out of time, should really try a smaller collection. @@@@ */
./code/policy.c:285:    sFoundation = (Size)0; /* condemning everything, only roots @@@@ */
./code/policy.c:286:    /* @@@@ sCondemned should be scannable only */
./code/policy.c:331:      if (res != ResOK) /* should try some other trace, really @@@@ */
./design/arena.txt:129:_`.req.attr.trans.time`: The translation shall take no more than @@@@
./design/arena.txt:140:more than @@@@ [something very small indeed]. (the non-obvious
./design/locus.txt:477:_`.arch.locus.attr`: contiguity, blacklist, zg, current region, @@@@
./design/locus.txt:487:@@@@.
./design/locus.txt:591:@@@@).
./design/object-debug.txt:73:(source @@@@).
./design/object-debug.txt:219:Alloc and Free methods. [@@@@alignment]
./design/object-debug.txt:260:_`.fence.slop`: [see impl.c.dbgpool.FenceAlloc @@@@]
./design/object-debug.txt:265:(`.req.tag.walk`_ and @@@@) into a generic facility for walking and
./design/arenavm.txt:103:Virtual Memory interface (see design.mps.vm_). @@@@ Explain why this
./manual/source/design/arenavm.rst:105:Virtual Memory interface (see design.mps.vm_). @@@@ Explain why this
./manual/source/design/locus.rst:478::mps:tag:`arch.locus.attr` contiguity, blacklist, zg, current region, @@@@
./manual/source/design/locus.rst:488:@@@@.
./manual/source/design/locus.rst:592:@@@@).
./manual/source/design/arena.rst:130::mps:tag:`req.attr.trans.time` The translation shall take no more than @@@@
./manual/source/design/arena.rst:141:more than @@@@ [something very small indeed]. (the non-obvious
./manual/source/design/object-debug.rst:74:(source @@@@).
./manual/source/design/object-debug.rst:220:Alloc and Free methods. [@@@@alignment]
./manual/source/design/object-debug.rst:261::mps:tag:`fence.slop` [see impl.c.dbgpool.FenceAlloc @@@@]
./manual/source/design/object-debug.rst:266:(:mps:ref:`.req.tag.walk` and @@@@) into a generic facility for walking and

[rptb1]

./manual/source/design/arena.rst:130::mps:tag:req.attr.trans.time The translation shall take no more than @@@@
./manual/source/design/arena.rst:141:more than @@@@ [something very small indeed]. (the non-obvious

This is an attempt to place a bound on a component of the critical path in the "second-stage fix". The author here knew that this was a critical step, but couldn't think of a way to express a quantitative bound. Today I looked at the assembly output on platform LII6LL (Linux, x86_64, Clang) and this step is about two-dozen instructions, but potentially O(log number-of-chunks) because of the chunk tree. Recently, we have measured the impact of the chunk tree on our commercial client and concluded that it's not causing any measurable impact, but nonetheless there is a scalability issue here.

But nonetheless, to eliminate this '@@@@' we need a way to express performance bounds on the stages of the critical path, and it's not even clear what units to use.

The unit that the client cares about is an overhead on their total runtime, so ultimately this should perhaps be expressed as a fraction of total runtime for a typical client application.