Skip to content

Latest commit

 

History

History
1794 lines (1207 loc) · 63.9 KB

review.rst

File metadata and controls

1794 lines (1207 loc) · 63.9 KB
Raw

Memory Pool System review procedure

tag:proc.review
Author: Richard Brooksby
Organization: Ravenbrook Limited
Date: 2023-01-19
confidentiality:public
Copyright: See C. Copyright and License
readership:MPS developers, [incorporate roles. RB 2023-01-20]
Status: draft

1. Introduction

This is the procedure for reviewing a change to the MPS in order to prevent defects (.purpose).

This procedure may seem overwhelming at first, but it can be executed quickly with practice.

Time to execute:

[TODO: Insert further examples and estimates from our records, especially recent experiences recorded in GitHub pull requests. RB 2023-01-30]

This procedure requires training, preferably by an experienced review leader (.role.leader.experienced). At the very least, do not apply this procedure to risky changes without first:

  • reading and understanding the whole document and the related rules
  • practising the procedure on low-risk changes

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119 [RFC-2119]. They indicate what is requried for the review procedure to succeed in achieving its .purpose.

This version of review was created as part of a project to migrate from Perforce to Git (and GitHub), and has details on conducting review via GitHub. The process is in no way specific to GitHub, Git, Perforce, or any other tool.

This procedure was created by incorporating and updating review process documents from Ravenbrook and the Harlequin MM Group (see A. References). This document is the result of process improvements from hundreds of reviews and thousands of hours of productive effort. The underlying process is largely derived from "Software Inspection" [Gilb-93], which was itself developed from Fagan inspection, and incorporates experience and measurement going back to the 1970s.

The process is an implementation of "Peer Review" (kpa.pr), a key process area of level 3 of the Capability Maturity Model [CMU-SEI-93-TR-025], but also contributes a great deal to:

  • Defect Prevention (kpa.dp, level 5)
  • Process Change Management (kpa.pcm, level 5)
  • Quantitive Process Management (kpa.qpm, level 4)

For further comparisons and criticism, see "Inspection and Other Review Techniques — a Comparison" [Gilb-93] §1.2.

2. Purpose

.purpose: The purpose of the review procedure is:

  1. .goal.fix: find and correct major defects
  2. .goal.prevent: prevent future defects

By doing this, review gets required changes into the MPS while protecting it from expensive defects (see .rationale).

.def.defect: A defect is a way in which the work does not meet its requirements.

.def.defect.major: A major defect is a defect that will probably have significantly increased costs to find and fix later in the development process (see .class.major).

As with any procedure, you MAY vary this one to meet this purpose, but you SHOULD read the .rationale.

3. Review Roles

.role: People taking part in review are given roles.

.role.two: All the roles MAY be covered by just two people. It's RECOMMENDED to have more people.

.role.all: The leader (.role.leader) MUST ensure that all roles are assigned to someone, to ensure that everything necessary gets done.

.role.everyone: Every person taking part in review SHOULD be assigned at least one role, to make it clear what they need to do.

.role.leader: The leader organises and plans the review, and ensures the procedures are executed. The leader is responsible for managing the process in all respects for productive results. The author (.role.author) MAY lead, but this NOT RECOMMENDED, and MUST be avoided if they are not .role.leader.experienced.

.role.leader.experienced: The leader SHOULD be experienced, scoring 24 or more points on "Self-Assessment Audit of Your Inspection/Review Process" [Gilb-93] pp xi-xv. In any case it is RECOMMENDED that the leader has received special instruction in review or inspection.

.role.author: The author is the person responsible for the change under review (.doc.product). For example, they're the developer who submitted a pull request. The author SHOULD read .advice.author.

.role.checker: A checker checks the change (.doc.product) during review. There MUST be more than one checker. Every person taking part in a review is usually also be a checker, including the author. Checkers will be asked by the leader to check with certain checking roles (.role.check) in mind; this is to increase coverage and reduce duplication of issues. Not every checker needs to understand everything about a change (e.g. the programming language) thoroughly. Many kinds of checking are useful. Checkers also take part defect prevention in .phase.brainstorm.

.role.editor: The editor is the person responsible for acting on the issues found during review in order to bring the work to review exit. It's RECOMMENDED that the editor and .role.author are the same person. For example, they're the developer who submitted a pull request and needs to fix it up before it can be merged.

.role.improver: The process improver takes action to prevent future occurrences of defects found by review. This SHOULD be the same person as .role.editor so that they maintain a good understanding of (and commitment to) process improvement and defect prevention, but this isn't always possible, e.g. when .role.author is an outside contributor.

.role.scribe: The scribe is the person who records information (not just issues) during review meetings. They are usually the same person as .role.leader. During .phase.check, review tools (such as GitHub) will often allow checkers to record issues as they check, in which case the scribe MUST ensure that this has been done. The scribe also records information during other phases, such as how much time a review took, who was there, who did what, etc.

4. Phases

.phase: This section describes the phases of a review. Each phase has a procedure. The phases involve varying groups of people (.role) and have diverse purposes.

.phase.handbook: This section may be used as a short "handbook" for people who have learned the procedure. (Compare with "A one-page inspection handbook" [Gilb-93].)

.phase.order: To review a change, the following procedures are executed roughly in the order below.

  1. .phase.request: .role.author requests that their change be reviewed. For example, they submit a GitHub pull request, or update the pull request state from "draft" to "ready to review".

  2. .phase.entry: .role.leader executes .entry. If the change doesn't meet the entry criteria then the change fails review, and the rest of the review process is not executed. A .role.author who is .role.leader.experienced MAY do entry on their own work.

  3. .phase.plan: .role.leader executes .plan to prepare the review and arrange for it to happen.

  4. .phase.kickoff: .role.leader and .role.checker execute .ko, beginning the review.

  5. .phase.check: .role.checker individually execute .check, according to their checking roles (.role.check), looking for unique major defects that no other checker will bring to the logging meeting. Checking continues during the next phase, .phase.log.

  6. .phase.log: .role.leader, .role.scribe, and .role.checker together execute .log to share and record what has been found, and to find more major defects, stimulated by what has been found so far. .phase.check continues during this phase.

  7. .phase.brainstorm: .role.leader, .role.scribe, and .role.checker, execute .brainstorm to come up with ways of preventing defects in future.

  8. .phase.estimation: .role.leader, .role.scribe, and .role.checker spend a few minutes using .calc to estimate how productive the review was, by:

    • estimating the cost of the review (mostly work hours)
    • projecting what the defects would cost if uncorrected
    • projecting what similar defects would cost if not prevented

    and .role.scribe records this information.

    [FIXME: There's nothing in the procedure sections about this. There needs to be at least a placeholder, so it doesn't get lost. RB 2023-11-09]

  9. .phase.edit: .role.editor executes .edit, analysing and correcting defects, but taking some action on every issue. This produces the revised change (.doc.rev).

  10. .phase.pi: .role.improver executes .pi to prevent major defects by correcting causes.

  11. .phase.exit: .role.leader executes .exit. If the revised change does not meet the exit criteria then it fails review. Otherwise it passes and may go on to be used, e.g. by being merged into the master codeline (proc.merge.pull-request).

Even the express review procedure (.express) has these phases.

5. Procedures

5.1. Review Entry

.entry: The review entry procedure MUST be executed when a change is submitted for review (.phase.entry).

.entry.purpose: The purpose of entry is to check whether the change is ready for review before planning a review, committing resources, organizing meetings, etc.

.entry.express: Does this change look low risk? Is someone available? Consider the express review procedure (.express).

.entry.record: Record the entry procedure (.doc.record).

  • On GitHub, you SHOULD start a comment on the pull request.

  • Record a the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.entry](https://github.com/Ravenbrook/mps/blob/d4ef690a7f2a3d3d6d0ed496eff46e09841b8633/procedure/review.rst#51-review-entry)

1. Start time 11:03.

.entry.change: Record exactly what the change is.

  • On GitHub, this information is implicitly recorded by commenting on the pull request in .entry.record.
  • Otherwise, record something like the branch name and commit hash. [Note: That's not really enough. See GitHub issue #273. RB 2023-11-09]

.entry.criteria: Determine and record the entry and exit criteria.

  • entry.universal and exit.universal always apply.

  • Add criteria for the types of documents altered by the change (code, design, etc.) from the procedure directory.

  • Record permalinks to the criteria. For example:

    Executing [proc.review.entry](https://github.com/Ravenbrook/mps/blob/d4ef690a7f2a3d3d6d0ed496eff46e09841b8633/procedure/review.rst#51-review-entry)

1. Start time 11:03.

2. Applying [entry.universal](https://github.com/Ravenbrook/mps/blob/eceaccdf5ab8d8614e9a8bb91a23bdcb99e7d0ce/procedure/entry.universal.rst) and [entry.impl](https://github.com/Ravenbrook/mps/blob/eceaccdf5ab8d8614e9a8bb91a23bdcb99e7d0ce/procedure/entry.impl.rst).

.entry.check: Check that the entry criteria hold. Record any transgressions. Decide whether to reject the change from review by balancing .purpose and cost. Will it pass .exit?

.entry.metrics: Record the time taken to execute .entry.

5.2. Review Planning

.plan: The review planning procedure MUST be executed when a change has passed .entry.

.plan.purpose: The purpose of planning is to prepare the review so that it is efficient and effective, and arrange for it to happen.

.plan.record: Record the planning procedure.

  • On GitHub, start a pull request comment.

  • Record the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.plan](https://github.com/Ravenbrook/mps/blob/d4ef690a7f2a3d3d6d0ed496eff46e09841b8633/procedure/review.rst#52-review-planning)

1. Start time 11:31.

.plan.iterate: Consider all of this procedure.

  • This procedure is only in rough order. Later steps may change earlier decisions. For example, availability of people for .plan.roles might affect .plan.tactics.

.plan.identify: Identify the change to be checked. This is often quite simply the documents altered by the change, but might need to expand to include dependent documents, or even things outside the project.

.plan.tactics: Examine the change and decide how to check it to achieve the .purpose.

  • The default and most effective tactic is to have .role.checker examine every line of the change, evenly distributing their attention by using a checking rate, such as 10 lines/minute.
  • Large repetitive automatic changes (search-and-replace) might be best handled by sampling using a random number generator and a strong Brownian motion producer (dice and tea).
  • Large changes might be broken up by document type, or topic, but you still want multiple .role.checker to look at everything.
  • Changes that cannot feasibly be checked (entry.universal.reviewable) may need to be reworked into stages, perhaps by version control transformations. (For example, the prototype branch/2014-02-19/remember-time was reworked into branch/2014-04-14/remember-time-2, branch/2016-03-22/remember-time-3, branch/2018-08-08/refset-struct etc.)
  • Record any variations from the default tactic.

.plan.time: Estimate the checking rate and time.

  • GitHub provides diff stats on the pull request (to the right of "Conversation").

  • .phase.check SHOULD last no more than one hour, so that checkers can maintain concentration.

  • .phase.log SHOULD last no more than two hours, so that checkers can maintain concentration.

  • It may be necessary to divide the review into multiple sessions.

  • Record your estimates. For example:

    Executing [proc.review.plan](https://github.com/Ravenbrook/mps/blob/d4ef690a7f2a3d3d6d0ed496eff46e09841b8633/procedure/review.rst#52-review-planning)

1. Start time 11:31.

2. proc.review.plan.time: About 500 lines of code @ 10 lines/minute
   so about 50 mins of checking.

.plan.schedule: Plan when this review may take place and who will attend. Negotiate with attendees if appropriate.

  • Record like:

    3. proc.review.plan.schedule: @thejayps and @UNAA008 will review
   2023-01-23 11:00 for about 2h.

.plan.train: Ensure that all participants are familiar with the review process.

  • Brief anyone new to the process about how it works and what is expected of them.
  • Ensure that they have the process documents.
  • Allow extra time for training.

.plan.source: Determine and record the source documents that could be used for checking (.doc.source).

  • Always include issues resolved or partially resolved by the change. There SHOULD be at least one (ensured by .entry.criteria).
  • Consider requirements, issues, designs, analysis, discussions, records of failures (e.g. in email messages), user documentation, standards.

.plan.rule: Determine and record the rules to apply (.doc.rule).

  • Add rules for the types of documents altered by the change (code, design, etc.) from the procedure directory.
  • Also select other rules that apply from the procedure directory, for example special rules that apply to the critical path. [These do not exist yet. RB 2023-11-09]

.plan.check: If there are relevant checklists available, determine and record which ones to apply.

.plan.roles: Decide and record the checking roles (.role.check) to assign.

  • Consider and try to assign every checking role (.role.check).
  • Choose checking roles that are most likely to find major defects in the type of change under review.
  • Always try to assign .role.check.backwards or a similar out-of-order sampling method, to help find defects in all parts of the change.
  • Bear in mind that .role.leader and .role.scribe will be somewhat occupied during logging and less able to check.
  • Assignments can be renegotiated in .ko.role.

.plan.homework: Assign work that people should do before the review.

  • Include background reading or other self-education that will help review efficiency. For example, reading about a technical aspect of the change.
  • You SHOULD NOT request review activities like studying source documents or looking at the change. Plan properly.
  • Ask anyone new to the process to read this document (see .plan.train).
  • Plan the review to succeed (but perhaps take longer) even if the homework is not done.

.plan.invite: Invite the checkers (.role.checker) to the kickoff meeting (.ko).

.plan.doc: Ensure that .role.checker have all the documents they need (the change, source documents, rules, etc.)

.plan.metrics: Record the time taken to execute .plan.

5.3. Review Kickoff

.ko: .role.leader holds the review kickoff meeting to ensure that the review begins, and that everyone involved has what they need to perform their roles.

.ko.record: Record the kickoff procedure.

  • On GitHub, start a pull request comment.

  • Record the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.kickoff](https://github.com/Ravenbrook/mps/blob/b2050e2cf69029fc13c31a724421945952d3fab2/procedure/review.rst#53-review-kickoff)

1. Start time 15:00.

.ko.doc: Ensure that every checker has all the documents they need.

.ko.intro: Optionally, ask the author for a short (one minute) introduction to the change.

  • Everyone should listen for new information this reveals. Start the .log.record early if there's anything that needs documenting, such as a hidden assumption or requirement. This happens!

.ko.remind: The leader reminds everyone:

.ko.role: Negotiate checking roles (.role.check).

  • .role.checker may volunteer for roles based on how they feel at the time. Focus and enjoyment are important for good results.
  • Ensure checkers understand their checking roles and checking rates.
  • Record who's doing what.

.ko.train: Offer private help to new .role.checker after .ko so that you don't delay .check.

.ko.improve: Announce any review metrics and negotiate review objectives.

  • Announce the rate.
  • Ask for suggestions or experiments with review procedure.
  • Record metrics and objectives.

.ko.log: Set a time for the logging meeting (.log).

  • This SHOULD be set at the estimated end of .ko, plus the estimated checking time (see .plan.time), plus a short break. Avoid delay.

.ko.author: Remind the author that they can withdraw the document from review at any time.

.ko.go: Send .role.checker away to start .check.

.ko.metrics: Record the time taken to execute .ko.

5.4. Review Checking

.check: The checking procedure SHOULD be executed by each individual .role.checker alone, carrying out their assigned checking roles (.role.check) without conferring with other checkers.

.check.purpose: The purpose of checking is to find unique major defects that no other checker will bring to .log.

5.4.1. Start

.check.doc: Ensure that you have all the documents you need to perform your checking role (.role.check).

.check.ask: Ask .role.leader if you have any questions about checking.

5.4.2. Checking

.check.record: You may note what you find in any way you like.

.check.record.start: Make a note of the start time.

.check.record.github: You SHOULD note issues using GitHub's pull request reviews in a way that will save time during .log.

.check.diff.not: You SHOULD NOT check using diffs unless your checking role says so. Check the work as it will be after the change only using the diffs to help direct attention.

.check.source: Read .doc.source for your .role.check.

  • Don't spend time searching for defects in .doc.source. If you happen to find any, that's a bonus. Note them for logging as .class.imp and possibly .class.major as well.

.check.rule: Ensure that you know .doc.rule and .doc.check.

  • If they've changed since you last read them, study and understand the changes.

.check.role: Ensure that you know .role.check and keep it in mind as you check.

.check.product: Check .doc.product.

.check.rate: Try to check at the planned checking rate (.plan.time). Do not rush. Slower is usually better. Control your attention.

.check.major: Concentrate on finding major defects.

.check.max: Find as many issues as possible to help the author.

.check.note: Note all issues; you need not log them later.

.check.rough: Your notes can be rough. .check.major!

  • Do not spend time making your issues neat and clear or even putting them in exactly the right place. Save that for .log. Search for more issues. .check.major!

.check.trouble: Consult .role.leader if you're having trouble:

  • you have questions
  • you are finding too many or too few issues

.check.class: Classify each issue you find (.class).

5.4.3. End

.check.metrics: At the end of checking, record

  • how many issues you found, by class (see .check.class)
  • how long you actually spent checking
  • how much of the product document you actually checked
  • any problems encountered

.check.metrics.github: You MAY record your metrics in a GitHub pull request review.

  1. Open the "Files changed" tab of the pull request.
  2. Click the green "Review changes" button.
  3. Enter metrics in the text box.
  4. Do not "finish" your review before .log to avoid distracting other .role.checker.

5.5. Review Logging

.log: The review logging procedure is executed by .role.leader and .role.scribe together with .role.checker.

.log.purpose: It has two purposes:

  1. to record issues for action
  2. to find more major defects, stimulated by sharing what has been found so far

.log.check: Checking continues during logging.

.log.advice: Remind the author of .advice.author.

.log.author: Remind the author that they can withdraw .doc.product from review at any time.

.log.record: .role.scribe_ MUST record the logging procedure.

  • On GitHub, start a pull request comment.

  • Record the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.log](https://github.com/Ravenbrook/mps/blob/12160d613b04045d6bd5380932f7560c91647556/procedure/review.rst#55-review-logging)

1. Start time: 15:50.

  • This opens .doc.log. .role.scribe MAY append issues to the log, but see .log.record.github.

.log.record.github: Ask .role.checker using the GitHub review tool to "finish" their reviews now, so that their notes and metrics are automatically included in .doc.log. Major defects recorded in this way MUST still be "logged" by announcing them to the meeting (.log.major).

.log.sums: "How many did you find?" Gather, sum, and record individual metrics from .check.record of:

  • how many issues were found, by class (see .check.class)
  • how long was spent checking
  • how much of the product document was checked

.log.decide: Now, and at intervals during logging, assess whether .doc.product is likely to pass .exit. If it seems very unlikely, consult with .role.author and .role.editor about aborting the logging meeting. Bear in mind:

  • Second reviews often find fewer issues, so it may be worth logging them anyway.
  • .brainstorm needs major defects to work on, and might prevent whatever went wrong here.
  • The MM Group never aborted logging.

.log.plan: Use the metrics to decide a logging rate.

  • The RECOMMENDED rate is at least one per minute. (This comes from MM Group experience. See [Gilb-93] §5.2.4 for detailed advice.)
  • Try to get all issues are logged during scheduled meeting time.
  • Slow down if many new issues are being found. Speed up if not. .role.checker will tell you when they find issues (.log.new).
  • Schedule breaks to maintain concentration.
  • Consider scheduling more logging meetings.

.log.scribe: Assign .role.scribe (usually the leader), and ensure .role.editor will find and be able to read the log.

.log.explain: .role.leader ensures .role.checker understand the order in which issues will be logged.

.log.format: .role.leader ensures .role.checker understand the desired form of issues, namely:

  • location
  • .class, including .class.new if the issue was discovered during logging
  • how it breaks which .doc.rule or .doc.check, if known, otherwise briefly what's wrong ("typo", "uninitialized", "obi-wan", "missing requirement", etc.)

.log.dup: .role.leader MAY remind .role.checker to avoid logging issues that have are duplicates of ones already logged.

.log.order: Ask .role.checker to try to list their issues in forwards document order. This makes life easier for other checkers and the editor. (There has been much experimentation with the order of logging, but was most effective the MM Group.)

.log.major: .role.leader calls upon .role.checker in turn to announce major defects they found.

.log.scribble: .role.scribe ensures that major defects are recorded so that they can all be found by .edit and .pi.

.log.fast: Log issues briskly. Allow people to clarify the issue, but discourage discussion. Encourage the search for more major defects. .role.leader should firmly discourage discussion of:

  • whether issues are genuine defects
  • how a defect may be resolved
  • the review process (other than to answer questions);
  • the answers to questions logged

.log.slow: Log issues slowly enough that .role.checker have time to understand issues and use them to find more major defects.

.log.new: When .role.checker find new major defects they should:

.log.decide.non-major: After logging major defects, decide whether and how many minor issues (.class.minor) to log during the meeting, considering .log.purpose.

.log.non-major: Go through .doc.product in sections (or equivalent), at each stage announce the section, ask who has issues, and request the issues.

.log.general: Ask .role.checker in turn for any general or new issues not already logged.

.log.brainstorm: Negotiate a time for the .brainstorm. This will normally be after a break at the end of .log.

.log.inform: Inform .role.editor that .doc.product is ready for .edit.

.log.metrics: Record the time taken to execute .log.

5.6. Review Brainstorm

.brainstorm: The review brainstorm procedure should be executed by .role.leader with .role.scribe and .role.checker very soon after .log.

.brainstorm.purpose: The purpose of review brainstorm is to come up with ways of preventing defects in future (.goal.prevent).

.brainstorm.time: The process brainstorm should last no more than around 30 minutes.

.brainstorm.record: Record the brainstorm procedure (.doc.record).

  • On GitHub, start a pull request comment.

  • Record a the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.brainstorm](https://github.com/Ravenbrook/mps/blob/branch/2023-01-19/review-procedure/procedure/review.rst#56-review-brainstorm)

1. Start time: 16:33.

.brainstorm.choose: Choose 3 to 6 major defects or groups of major defects found in review.

  • Make this choice based on defect importance and your experience of which defects can be most profitably attacked.
  • Record the issues, e.g. by pasting links into the record comment, so you can edit them for .brainstorm.log.

.brainstorm.remind: Remind everyone of .brainstorm.purpose and .pi.scope.

.brainstorm.focus: Ask everyone not to spend time analysing the defects found by the review, or suggesting ways to fix those defects, except insofar as it is necessary to develop ways to prevent those defects.

.brainstorm.raise: Raise each major defect in turn, reminding participants of the issue, and asking how it happened and what could have prevented it.

.brainstorm.disc: Encourage discussion for no more than about five minutes per defect. Focus on how the defect arose, and what improvement could prevent it. Curtailing discussion of how the defect might be fixed.

.brainstorm.new: If .role.checker find new major defects do the same as .log.new.

.brainstorm.proc: If time permits, the leader may solicit criticisms of the review process and apply .brainstorm.disc to them.

.brainstorm.log: Record the suggestions like:

2. For https://github.com/Ravenbrook/mps/pull/117#discussion_r1090530823
   : @thejayps suggests a checklist item perhaps, where you
   deliberately try to misinterpret your sentences and improve
   them if you can (misinterpret them).

.brainstorm.pending: If you record a suggestion, ensure that the pull request is labelled "pending" so that it doesn't get forgotten when the pull request is closed. These are picked up during regular management of pull requests by proc.regular.

.brainstorm.metrics: Record the time taken to execute .brainstorm.

5.7. Review Edit

.edit: The review edit procedure MUST be executed by .role.editor to revise .doc.product into .doc.rev by processing .doc.log.

.edit.purpose: The purpose of the review edit is to analyse and correct defects, part of the review's primary purpose (.goal.fix).

.edit.record: Record the edit procedure.

  • On GitHub, start a pull request comment.

  • Record the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.edit](https://github.com/Ravenbrook/mps/blob/f8b6c94be9304d017d8a5cf57f7f4ab367ac51fc/procedure/review.rst#57-review-edit)

1. Start time 2023-02-02 09:45.

.edit.record.time: Edit might take several sessions. Keep track of the working time spent for .edit.metrics.

.edit.read: Locate and read all of .doc.log before making any edits.

.edit.log: Record your actions in one of these ways (in order of preference):

  • Respond to the issue like a conversation. This works well for GitHub review conversations.

  • Quote the text of the issue in a comment. This works well for issues in pull request comments.

  • Edit the log and record your action in a comment, e.g.

    m: Warthog too warty.  [Fixed: Warts reduced in f93b75dc]

  • Append your action to the .edit.record with a reference.

  • In any case, your actions MUST be recorded permanently in a way that is traceable from .doc.log.

.edit.act: You MUST take action on every issue in .doc.log and record that action. Record one of the following responses:

.edit.act.fix: Fix the defect and say a few words about how. Always say where.

  • Write "Fix: <how> in <commit>"

.edit.act.reject: Reject the issue with a reason why it is not a valid issue.

  • Write "Reject: <reason>"

.edit.act.comment: Add a comment to .doc.product rather than "fixing" the issue. Say why the issue cannot be fixed. Note that this is not the same as fixing a defect in a comment.

  • Write "Comment: <reason> in <commit>"

.edit.act.raise: Escalate for later action, usually by creating an issue to go into the project queue, such as a GitHub issue.

  • Write "Raise: <reference>"
  • This can apply to .class.question if it a difficult one.

.edit.act.forget: Decide that the issue is not worth an action, even though it's valid. Give your reason.

  • Write "Forget: <reason>"
  • Use with caution, and never for .class.major.

.edit.act.answer: For .class.question, give an answer, and tag or message the questioner so that they see it.

  • Write "Answer: <answer>"
  • You MAY send an answer by some other traceable means and link it.

.edit.act.imp: Pass the issue to another person, and ensure they accept it.

  • Write "Pass: <person>"
  • Mainly intended for .class.imp, where some outside document needs an edit.

.edit.extra: You may make corrections to defects which you spot yourself during editing work. Log them like those found during .check or .log and inform .role.leader about them.

.edit.exit: After action has been taken and recorded on every logged issue, tell .role.leader that the revised change is ready for .exit.

.edit.metrics: Record both the working time spent and the end time of .edit.

5.8. Process Improvement

.pi: The process improvement procedure MUST be executed by .role.improver to take action to prevent future defects by processing .doc.log, but especially the results of .brainstorm.

.pi.purpose: The purpose of process improvement is to take action to prevent future defects, closing the process improvement loop (.goal.prevent).

.pi.scope: The scope of actions that might be taken by the improver should not be limited, and could include:

  • filing process issues for later action
  • changing a personal habit
  • raising concerns with management
  • sending suggestions to anyone
  • suggesting wholesale review of working practices
  • requesting training for staff.

as well as changes like:

.pi.record: Record the process improvement procedure.

  • On GitHub, you start a pull request comment.

  • Record the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing [proc.review.pi](https://github.com/Ravenbrook/mps/blob/f8b6c94be9304d017d8a5cf57f7f4ab367ac51fc/procedure/review.rst#58-process-improvement)

1. Start time 2023-02-02 11:45.

.pi.record.time: See .edit.record.time.

.pi.read: Locate and read all of the suggestions recorded in .brainstorm.log before making any decisions.

.pi.log: Record your actions in the same manner as edit actions (.edit.log).

.pi.act: You MUST take a written action for every improvement suggestion and record that action. Record your response like an edit (.edit.act).

.pi.exit: After action has been taken and recorded on every suggestion, tell .role.leader.

.pi.metrics: See .edit.metrics.

5.8. Review Exit

.exit: The review exit procedure is MUST be executed by .role.leader after editing (.edit).

.exit.purpose: The purpose of exit is to determine whether the revised change passes review.

.exit.record: Record the exit procedure (.doc.record).

  • On GitHub, you start a pull request comment.

  • Record a the procedure you're following (this one) and the start time. Use a permalink. For example:

    Executing (proc.review.exit)[https://github.com/Ravenbrook/mps/blob/645200a25e5e415a2a2978d550b5251e0284c43e/procedure/review.rst#58-review-exit]

1. Start time 10:20.

.exit.check: Check that the exit criteria hold (see .entry.criteria).

  • Record any transgressions, like:

    2. exit.universal.quest: Question 5 answered in chat but not in docs.

.exit.fix: Fix transgressions, if it is feasible with low risk. Otherwise ask .role.editor to fix them. Record this action, and record edits in the same way as .edit.

.exit.fail: If transgressions remain, then the revised change is too defective. It fails review and MUST NOT be used.

  • Record this result, like:

    3. Revised change rejected.

  • Tell project management, because they need to decide how to handle the situation.

.exit.pass: Otherwise, the revised change passes review and the resulting .doc.acc MAY be used.

  • Record this result, like:

    3. Revised change passed.

  • On GitHub, approve the pull request for merge.

  • Tell the person who will deploy .doc.acc, such as someone who will merge it to master.

.exit.calc: Calculate and record final review metrics (.calc). For example:

4. review.exit.calc:
   - hours used: 11
   - hours saved: 70
   - major defects remaining: 1.5

.exit.inform: Inform all review participants of the result of their efforts.

.exit.metrics: Record the time taken to execute .exit.

6. Documents

[Sourced from [MM-process.review] and needs updating. RB 2023-01-21]

.doc: The review process involves a lot of documents. This is a brief explanation of what they are.

.doc.forms: Documents come in many forms. They might be web pages, email messages, GitHub comments, chat messages, and sometimes even printed on dead trees.

.doc.source: Source document

A document from which the product document is derived. Note that this does not mean "source code".

For example, a failure of the software might result in a failure report, which gets logged to an issue, where someone writes an analysis and designs a solution. All of those things are source documents for the resulting change to be reviewed (.doc.product).

Other examples include .doc.guide, manuals, and standards.

.doc.product: Product document

The document developed from the source documents, and offered for review. The work under review. The changes under review. The work product.

Often, we are reviewing a change to the MPS, in the form of a branch to be merged in order to meet a requirement. In this case, the product is technically the change, and yet we focus the review on the MPS as it will be after the change, in order to find defects that would be introduced (see also .plan.identify). The admonition .check.diff.not is a manifestion of this principle.

.doc.record: Review records

Documents produced by the review procedures, which record the progress and results of the review. See .entry.record, .plan.record, .ko.record, .check.record, .log.record, .brainstorm.record, .edit.record, .pi.record, and .exit.record.

On GitHub, the record of a review SHOULD consist of pull request comments, pull request reviews, and line comments. See also .doc.log.

In any case, review records SHOULD be specific, permanent, and MUST be referencable.

.doc.log: Issue log

A record of issues raised during the logging meeting, specifying their location, type, finder, and a brief description.

On GitHub, the issue log includes all GitHub pull request reviews or pull request comments for the change under review. See also .doc.record.

Every issue log entry SHOULD be specific, permanent, referencable, and MUST be traceable from .doc.product and .doc.rev.

.doc.rev: Revised document
The result of performing the edit procedure on the .doc.product. The revised version of the change under review.
.doc.acc: Accepted document
The result of a Revised document passing exit.
.doc.rule: Rules and rule sets

A rule or set of rules that .doc.product should obey.

Rules are developed by process improvement of the project as a whole. In this procedure, they are updated by .pi as a result of .brainstorm.

Rule sets are kept short and and rules kept terse to help with checking.

.doc.guide: Guides

A guide that .doc.product is expected to follow, though not strictly.

Guides are generally longer, more detailed, and more discursive than .doc.rule and contain advice about good practice. As such, they are less useful for review checking than .doc.rule or .doc.check.

Guides are developed by process improvement of the project as a whole. In this procedure, they are updated by .pi as a result of .brainstorm.

.doc.check: Checklists

A list of questions to help check against .doc.rule. A negative answer to a checklist question indicates that a rule has been broken.

Checklists often contain specific questions that can help determine whether rules are broken. For example, a code checklist might say

.error.check: Are function status/error/exception returns checked and acted upon?

which is ultimately part of a checking generic rule like

.achieve: A document must achieve (be consistent with) its purpose.

Checklists are developed by process improvement of the project as a whole. In this procedure, they are updated by .pi as a result of .brainstorm.

.doc.entry: Entry criteria
.doc.rule that SHOULD be met before review to ensure that the .doc.product is likely to pass .doc.exit, so that resources are not wasted on a premature review.
.doc.exit: Exit criteria
.doc.rule that SHOULD be met for .doc.rev to pass review and be approved for use.
.doc.proc: Procedures
Descriptions of the steps involved in completing any part of process (development, review, or otherwise).
.doc.imp: Brainstormed improvement suggestions
Suggested improvements to process (and hence to some document) arising from the process brainstorm.
.doc.request: Requests for change
An issue that the editor cannot deal with that is escalated to some other tracking system, such as a GitHub issue.

7. Calculations

[Update the gender-specific tags in this section. RB 2023-02-02]

.calc: [Need to mention how this info is used. Ref kpa.qpm. RB 2023-01-26]

.calc.manpower-used: The manpower used is the time for entry, kickoff, checking, logging, brainstorm, edit, and exit. Kickoff, checking, logging and brainstorm should be multiplied by the number of checkers. Entry and kickoff may be assigned to another document reviewed at the same time.

.calc.manpower-saved: The default calculation is the number of major defects found and fixed, multiplies by 10 man-hours. This represent the cost of a major defect found by QC. If the defect would have reached customers, the estimate should be 100 man-hours. A better estimate can be made, with justification.

.calc.defects-remaining: The calculation of defects remaining should use the estimate <major defects found>/<number of pages>. The obvious adjustment should be made for sampling. The number of unresolved major issues (raised) should be added. [In an ideal world, I believe we should know what proportion of major defects we find, and use that. Perhaps we could use 75%? - GavinM] [Doesn't that mean we could determine whether a document fails review before .edit? RB 2023-01-28]

8. Checking Roles

["Checking role" is too easily conflated with "review role" and should perhaps be renamed to "method". RB 2023-01-23]

.role.check: Checking roles are assigned (.plan.roles) to .role.checker in order to focus their attention on different aspects of the change under review, and so increase the number of unique major defects found.

.role.check.backwards: The backwards checking role involves scanning the product document in reverse order, in order to increase the chances of finding major defects that won't be found by other checkers. The checker should use their initiative in determining the granularity of this reversal; for example: in an implementation, the checker might read each function or type definition in turn from the end of the file; for other documents, the checker might read each subsection or paragraph from the end backwards. For the convenience of other checkers and the editor, the backwards checker should their issues in forwards document order. See .log.order. [This advice may no longer be relevant with automated tools. RB 2023-01-26]

.role.check.clarity: The clarity checking role focuses on whether the product document is clear and obvious. This is a good role to give to someone who has never seen the product document before, but who is in the intended readership. Anything that is unclear to them is a defect.

.role.check.consistency: The consistency checking role focuses on whether the product document or documents are internally consistent.

.role.check.convention: The convention checking role concentrates on whether the product document complies with detailed conventions and rules.

.role.check.correctness: The correctness checking role focuses on whether the product document is correct, i.e. will have the intended consequences.

.role.check.source: The source checking role concentrates on whether the product document is consistent with any source documents, and whether dependencies and links are documented where appropriate.

[Other possible checking roles:

  • checking using a different medium (printouts)
  • checking random things in a random order, using dice
  • sampling large or repetitive changes at random
  • build, test, lint, and other automated tools

RB 2023-01-29]

9. Issue Classification

[Imported from mminfo:guide.review.class and needs updating. RB 2023-01-26]

.class: There are many possible schemes for defect classification, but only a coarse one is used here. Any issue raised, should fall into one of the following classes. The normal abbreviation is indicated.

.class.major: (M): A Major defect is a defect in the Product document that will probably have significantly increased costs to find and fix later in the development process, for example in testing or in use ([Gilb-93] p442). A bug that is fixed after review typically takes one man-hour, after testing 10 man-hour, and in the field 100 man-hours; and it's much worse for a garbage collector (.rationale.gc). A defect that will waste downstream development effort is also major. Typical major defects are:

  • In an implementation, potentially failing to behave as specified;
  • In an implementation, failing to validate foreign data;
  • In a high-level document, being likely to cause major defects in derived documents.

.class.minor: (m): A minor defect is any defect in the Product document whose cost to fix does not increase in time. If there is a typo, then it doesn't matter when it's fixed. Typical minor defects are:

  • an implementation, poor variable names;
  • in any human-readable text, typos where the meaning is clear.

.class.comment: (C): A comment is any remark about the product document. Typical comments are:

  • suggestions for how an algorithm could be optimised in future;
  • praise.

.class.question: (q): A question is any matter on which .role.checker wants clarification.

  • If a product document is unclear to the intended readership then that's also .class.major or .class.minor, by rule.generic.clear.
  • Questions will be answered in writing (.edit.act.answer). Answering them often spawns changes anyway.
  • Typical questions are:
    • Clarifications on why things should be the way they are;
    • Curiosity about the details of something.

.class.imp: (I): An improvement suggestion is any potential defect found in documents other than the product document. Typical improvement suggestions are:

  • defects in source documents;
  • defects in rule sets, check lists, or procedures.

.class.new: (N): Any issue found during logging (as opposed to during checking) is a new issue. This classification is orthogonal to the preceding. It is important to mark new issues, in order to measure how worthwhile group logging sessions are (see .log.purpose).

11. Advice for the author

.advice.author: The intense scrutiny a formal review of your work can be distressing. Remember that you are not under attack. Everyone is working to make your work better.

With that in mind, here is some advice from [Gilb-93]:

  • Report your own noted issues after giving your team-mates a chance.
  • Don't say 'I found that too!'
  • Thank your colleagues for their efforts on your behalf.
  • Learn as much as possible about avoiding the issues as an author.
  • Respect the opinion of your team-mates. Do not justify or defend.
  • Check the logging for legibility and intelligibility.
  • Answer any 'questions of intent' logged by checkers at the end of the logging meeting.

12. Express review

.express: The express review procedure [RB-2023-02-01] MAY be executed by .role.leader.experienced to get a low-risk change reviewed quickly, at low cost.

.express.readership: The readership of this section is .role.leader.experienced.

.express.brief: If anything in this section is unclear to you, you're not ready to run express reviews.

.express.try: During an express review, if things go wrong, or turn out to be riskier or more complicated than you thought, just go back and .plan a full review. Record that you did. Don't delete the express review record.

.express.record: Record the express procedure (.doc.record). You MAY squash the records for the other steps in one comment.

.express.entry: Execute .entry pretty much as usual.

.express.call: Call someone else in right now.

.express.risk: The other person MUST agree that the change has low risk, and that express review will achieve the .purpose.

  • Size is not risk. It's much more important to consider what is being changed and how.

.express.time: Express review should take no more than about 30 minutes. If it takes longer, revert to full review.

.express.schedule: No need to schedule. You both do it now.

.express.train: Choo choo! Don't do this with untrained people. Revert to full review.

.express.source: All source docs MUST be immediately available. If not, you know what to do by now.

.express.rule: Everyone SHOULD know the relevant rules.

.express.homework: If homework is needed, it's not an express review.

.express.remind: Remind everyone of the .purpose of review.

.express.role: Everyone will perform every .role.check. Not feasible? It's not an express review.

.express.improve: Express reviews don't support extra objectives.

.express.major: If anyone finds major defects, stop the express review and .plan a full one.

.express.save: Save the review record before you check by submitting the review record comment. GitHub has a tendency to lose the draft if you start using your browser for checking.

.express.check: Do separate checking for some minutes. Look for major defects, note other issues. Don't confer.

.express.log: Confer. Announce issues, look for major defects, note other issues.

.express.log.proper: You still need to record issues properly, even in an express review. Don't know how? You're not ready to run an express review.

.express.brainstorm: Take a one minute break after logging then do a few minutes of brainstorm. Prevention is still a goal.

.express.edit: If there are just a few minor edits, do them now, together (like pair programming). Otherwise, drop out of express review into .edit. Record this decision, natch.

.express.pi: Defer/delegate .pi but don't drop it. Prevention is worth it.

.express.exit: Execute .exit pretty much as usual. Do record metrics.

13. Rationale

.rationale: Formal review is the key to the quality of the Memory Pool System.

A full justification of the review process described by this procedure is not feasible here. There are three sources:

  1. the process improvement history of the Memory Pool System project,
  2. Software Inspection [Gilb-93],
  3. the analysis work behind the Capability Maturity Model [CMU-SEI-93-TR-024].

Of these, (1) is unfortunately the least accessible, because the documents have travelled through several different systems, and version control did not always survive.

Ravenbrook does have hundreds of archived review records [MM-reviews] with estimates of review productivity (produced by .phase.estimation). [At some point it would be good to summarize those here. RB 2023-01-28]

13.1. Why formal reviews?

Every formal review has been worthwhile in terms of preventing defects versus the cost of review.

The Harlequin MM Group adopted code review in the mid 1990s -- early compared to most of the industry. Casual code reviews (where someone eyeballs diffs) have become standard practice for many projects, and it's quite hard to imagine a time without them. However, full-on formal reviews or inspections are still relatively rare.

.rationale.gc: Formal review is appropriate for the MPS because defects in memory managers, and especially in garbage collectors, are extremely expensive to find and fix compared to other software.

It's the job of a garbage collector to destroy information by recycling (overwriting) objects and reorganizing memory. A subtle failure of GC logic can cause a failure in the client software many hours later. When that failure happens to a user of an application delivered by developers using a compiler developed by your client that uses the MPS in its runtime system, well, forget about it. A defect in the compiler (usually considered expensive) is relatively cheap!

This means that the cost of major defects escalates much more steeply for the MPS than most software, so it is especially worthwhile to catch them early in the development process.

Even testing is too late.

A. References

[CMU-SEI-93-TR-024]"Capability Maturity Model for Software, Version 1.1"; Mark C. Paulk, Bill Curtis, Mary Beth Chrissis, Charles V. Weber; Software Engineering Institute, Carnegie Mellon University; 1993-02; <https://resources.sei.cmu.edu/library/asset-view.cfm?assetid=11955>.
[CMU-SEI-93-TR-025]"Key Practices of the Capability Maturity Model, Version 1.1"; Mark C. Paulk, Charles V. Weber, Suzanne M. Garcia, Mary Beth Chrissis, Marilyn Bush; Software Engineering Institute, Carnegie Mellon University; 1993-02; <https://resources.sei.cmu.edu/asset_files/TechnicalReport/1993_005_001_16214.pdf>.
[Gilb-93](1, 2, 3, 4, 5, 6, 7, 8, 9) "Software Inspection"; Tom Gilb, Dorothy Graham; Addison Wesley; 1993; ISBN 0-201-63181-4; book.gilb93.
[MM-guide.review.edit]"Guidelines for review edits"; Gavin Matthews; Harlequin Limited; 1996-10-31; mminfo:guide.review.edit; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/guide/review/edit/index.txt#1.
[MM-process.review]"The review process"; Richard Brooksby; Harlequin Limited; 1995-08-18; mminfo:process.review; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/process/review/index.txt#1.
[MM-proc.review.brainstorm]"Procedure for process brainstorm in review"; Gavin Matthews; Harlequin Limited; 1997-06-12; mminfo:proc.review.brainstorm; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/brainstorm/index.txt#1.
[MM-proc.review.check]"Procedure for checking in review"; Gavin Matthews; Harlequin Limited; 1997-06-12; mminfo:proc.review.check; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/check/index.txt#1.
[MM-proc.review.entry]"Procedure for review entry"; Gavin Matthews; Harlequin Limited; 1997-06-02; mminfo:proc.review.entry; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/entry/index.txt#1.
[MM-proc.review.exit]"Procedure for exiting a document from review"; Gavin Matthews; Harlequin Limited; 1997-06-12; mminfo:proc.review.exit; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/exit/index.txt#1.
[MM-proc.review.ko]"Procedure for a review kickoff meeting"; Gavin Matthews; Harlequin Limited; 1997-06-12; mminfo:proc.review.ko; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/ko/index.txt#1.
[MM-proc.review.log]"Procedure for review logging meeting"; Gavin Matthews; Harlequin Limited; 1997-06-12; mminfo:proc.review.log; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/proc/review/log/index.txt#1
[MM-reviews]Review records of the MM Group; Harlequin Limited; mminfo:review.*; //info.ravenbrook.com/project/mps/doc/2002-06-18/obsolete-mminfo/mminfo/review/...
[RB-2023-02-01]"Express review notes and test"; Richard Brooksby; Ravenbrook Limited; 2023-02-01; <https://info.ravenbrook.com/mail/2023/02/01/20-06-44/0/>.
[RFC-2119]"Key words for use in RFCs to Indicate Requirement Levels"; S. Bradner; Harvard University; 1997-03; <https://www.ietf.org/rfc/inline-errata/rfc2119.html> (with errata).

B. Document History

2023-01-19 RB Created.
2023-01-20 RB Importing material from MM Group proc.review.
2023-01-26 RB Importing checking roles and issue classification from MM Group documents.
2023-01-28 RB Developing the Rationale. Tidying up remaining comments. Revising entry, planning, kickoff, and exit. Revising documents section.
2023-01-30 RB Revising checking, logging, and brainstorm.
2023-01-31 RB Revised based on review test run.
2023-02-01 RB Implementing .express.
2023-02-06 RB Checking against and referencing [Gilb-93].
2023-11-09 RB Updating prior to reviewing the review to fill in holes based on recent experience.

C. Copyright and License

Copyright © 2023 Ravenbrook Limited.

Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:

  1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
  2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.