4089

Clean up how we reclaim local scopes.

It used to work like this (commit 3216):

  1. Update refcounts of products after every instruction, EXCEPT:

      a) when instruction is a non-primitive and the callee starts with
      'local-scope' (because it's already not decremented in 'return')

    OR:

      b) when instruction is primitive 'next-ingredient' or
      'next-ingredient-without-typechecking', and its result is saved to a
      variable in the default space (because it's already incremented at
      the time of the call)

  2. If a function starts with 'local-scope', force it to be reclaimed
  before each return. However, since locals may be returned, *very
  carefully* don't reclaim those. (See the logic in the old `escaping`
  and `should_update_refcount` functions.)

However, this approach had issues. We needed two separate commands for
'local-scope' (reclaim locals on exit) and 'new-default-space'
(programmer takes charge of reclaiming locals). The hard-coded
reclamation duplicated refcounting logic. In addition to adding
complexity, this implementation failed to work if a function overwrites
default-space after setting up a local-scope (the old default-space is
leaked). It also fails in the presence of continuations. Calling a
continuation more than once was guaranteed to corrupt memory (commit
3986).

After this commit, reclaiming local scopes now works like this:

  Update refcounts of products for every PRIMITIVE instruction.
  For non-primitive instructions, all the work happens in the `return`
  instruction:
    increment refcount of ingredients to `return`
      (unless -- one last bit of ugliness -- they aren't saved in the
      caller)
    decrement the refcount of the default-space
      use existing infrastructure for reclaiming as necessary
      if reclaiming default-space, first decrement refcount of each
      local
        again, use existing infrastructure for reclaiming as necessary

This commit (finally!) completes the bulk[1] of step 2 of the plan in
commit 3991. It was very hard until I gave up trying to tweak the
existing implementation and just test-drove layer 43 from scratch.

[1] There's still potential for memory corruption if we abuse
`default-space`. I should probably try to add warnings about that at
some point (todo in layer 45).

1 files changed, 9 insertions, 0 deletions

diff --git a/062convert_ingredients_to_text.cc b/062convert_ingredients_to_text.cc
index cf639b9e..abbacb1e 100644
--- a/062convert_ingredients_to_text.cc
+++ b/062convert_ingredients_to_text.cc
@@ -143,6 +143,15 @@ bool is_static_array(const reagent& x) {
   return !x.type->atom && x.type->left->atom && x.type->left->name == "array";
 }
 
+//: Supporting 'append' above requires remembering what name an instruction
+//: had before any rewrites or transforms.
+:(before "End instruction Fields")
+string name_before_rewrite;
+:(before "End instruction Clear")
+name_before_rewrite.clear();
+:(before "End next_instruction(curr)")
+curr->name_before_rewrite = curr->name;
+
 :(scenarios run)
 :(scenario append_other_types_to_text)
 def main [