Container handle processing and cycle detection changes in the 8.1 garbage collector

[MichaelJCaruso]

Building a bootstrap database on the Mac uncovered a bug in the release 8.1 version of the garbage collector. Note that this is a logic issue and NOT a MacOS specific problem; however, it does not manifest itself in similar fashion on other platforms. In all likelihood that is the result of timing differences in the re-use of reclaimed memory but that hypothesis has not been confirmed.

Current Status

The goal of this pull request is to address this and whatever other issues prove to be related. Currently, the changes found here do not yet do that. They do, however:

• re-factor traversal processing code (d03a7c30a5f7d3c5c46a7726eafe73e272f3817b) in anticipation of expected changes to come.
• add environment variable controlled logging (4515e6441430141fa855bdf14f0ca3193a70332b) of key container handle processing events.
• rename and encapsulate routines involved in handle visit processing (also 4515e6441430141fa855bdf14f0ca3193a70332b).
• fix an incorrect predicate used to determine when a handle attached to a container table entry is only being cached for possible future use (4ebea79525be4428c3e61912674a72619c8472c4).
• emancipate a surprisingly unemancipated CPD (42fea953a9f76e564d011130d26c06d4e4afa9b9).
• perform a number of other code cleanups (e.g., 93b8aefbf0cf1ef8dd7abf91e9447e441251a642).

Relative to the primary bug fix goals of this request, the following changes are currently present as well:

• 82d6e61cc84735f3879eb55278779685f80646dd fixes a bug that was over-counting references to container handles during the handle cycle detect phase of the garbage collector.

Problem Analysis

In a nutshell, it appears that the 8.1 garbage collector is failing to retain containers indirectly referenced by handle alone. The following structure exposes the problem:

   (Vector)
      |
      |     ... POP via USegStore
      |
      V
  (Closure)
      |
      |     ... Handle via Context
      |
      V
  (Context) ... only reference is from Closure handle
      |
      |     ... Handle via Descriptor Store
      |
      V
   (Store)  ... only reference is from Context handle

There are a couple of different things going on that jointly conspire to cause a problem:

• Handle based retention is based solely on logic in the 8.1 garbage collector that attempts to determine if a handle is referenced from outside the mesh of containers. There is no provision in the 8.1 garbage collector to directly mark a container for retention based on references from other handles.
• The discovery algorithm is essentially a one pass algorithm that relies on a seed set of container table entries reachable only by container handle. At the conclusion of that algorithm's execution, any container (handle) that proves reachable exclusively from one of the handles in the initial seed set is considered unreachable and therefore reclaimable. There is no provision for re-running the discovery algorithm excluding container table entries originally in the seed set that prove to be reachable based on the algorithm's result (although that could possibly be sufficient by itself to correct the problem).

Without going into too much detail regarding the discovery algorithm, here's what appears to be happening:

• The POP reference from (Vector)->(Closure) keeps (Closure) alive and keeps (Closure) from appearing in the initial seed set.
• Because there are no POP references to either (Context) or (Store), they are both candidates for inclusion in the discovery algorithm's seed set.
• Because (Closure) is not in the seed set, the discovery algorithm correctly concludes that (Context) is externally referenced and must be preserved. (Store), unfortunately, does not share that fate. Because (Context) was in the seed set, the discovery algorithm finds the one and only reference to (Store) inside the container mesh. Based on that discovery, it concludes that (Store) can be reclaimed despite the fact that it just proved that (Context) should not have been in the seed set in the first place.

[VCommitter]

Crud. This cycle garbage collection was introduced because it fixed a memory leak in the datafeeds that rendered Interface BatchFeedManager's batching ineffective. Without the cycle detection the batching actually doesn't conserve memory; without the cycle detection uploads using Interface BatchFeedManager have the same memory consumption as if it weren't used.

We can put this into master as an incremental step but I hesitate to put it into a release branch; it will break most necessary uses of Interface BatchFeedManager.

[MichaelJCaruso]

Could not know that, but that sounds like a great production test case.

[beepoxis]

Now that you mention it, I'm almost certain I baked a reproducer into the ivr test suite

[VCommitter]

Does this pull request allow bootstraps to be built where they otherwise fail to build? Or does it succeed with a warning?

[MichaelJCaruso]

The former. Without it, bootstrap fails. With it, bootstrap quietly and correctly succeeds.

[VCommitter]

Is this meant to be a "pure" refactor now? I didn't see the mark() -> startMark() change to the Cycle Detection visitor so I thought you might be pull requesting the refactor that isn't meant to change or fix functionality?

[MichaelJCaruso]

This is a pure re-factor at the moment with no functionality changes except for the two bug fixes mentioned in the updated pull request. I'll submit the change you mention now.