Context garbage collection

[redlizard]

Implements garbage collection of Contexts, addressing issue #3174.

The essence is a mark and sweep garbage collection algorithm. It doesn't keep track of roots; instead, it keeps track of all existent contexts, and when performing a sweep, it computes which contexts only have inbound references from other contexts, and those that do not are considered roots. From there a standard reachability and deletion are done.

Running time of a garbage collection run is proportional to the total size of all allocated values in the managed contexts. This consists of context variables, but also of elements of vectors in those context variables. Garbage is collected whenever this total size gets too big; to that end, vector values do some bookkeeping to keep track of this total size. Which is a bit messy, but necessary.

Depends on #3764.

[lgtm-com]

This pull request introduces 4 alerts and fixes 2 when merging d6cf5c0 into 19cca81 - view on LGTM.com

new alerts:

• 3 for Catching by value
• 1 for Missing return statement

fixed alerts:

• 2 for Resource not released in destructor

[lgtm-com]

This pull request fixes 2 alerts when merging 0ac786d into 19cca81 - view on LGTM.com

fixed alerts:

• 2 for Resource not released in destructor

[rcolyer]

I reviewed this code and it looks pretty solid. The core GC algorithm was a little too much for me to validate properly in the time I had available, but everything else looks good, and the structure is solid. I ran tests validating that it fixes the leak on simple and harder cases. The performance is very comparable (within 2-4%) on simpler computational code. I found an edge case L-system generator which is about 25% slower, which @redlizard has for further benchmarking.

I think the performance edge case warrants some more careful inspection first to either come to terms with the scope of how many cases this applies to, or resolve it. Other than that, this seems ready to me as a fix. Solid work @redlizard.

[redlizard]

The edge case identified by @rcolyer seems to be a missed optimization opportunity that particularly affects highly recursive designs. This will be significantly easier to improve once I'm done with some unrelated things I'm working on, at which point I'll see how much there is to win here.

[redlizard]

On actually trying it, turns out these optimizations don't need to wait for any unrelated work, so they're in.

Optimizing heap size accounting logic while avoiding using the garbage collection machinery for situations that don't need it get rid of somewhere between 50% and 75% of the unnecessary overhead in the example provided by @rcolyer. The remainder seems hard to avoid -- most of it is the cost of keeping track of to-be-collected contexts in the garbage collector, which is nontrivial for some reason I don't fully understand. I don't think this can be avoided in any easy way; maybe a solution along the lines of an optimized allocator for contexts could optimize this further, but I'm not sure that's worth the maintenance hassle.

[rcolyer]

@redlizard This is an undefined behavior warning that cannot be ignored. NDEBUG is defined during release builds, which disables the assert on line 378, which means the else case is not actually handled (a maintenance issue if extra types arise there). If you want to keep the assert, following the assert it should either return null (the subsequent Qt connect will print an error to console and then continue having connected nothing) or throw an exception.

./src/parameter/ParameterWidget.cc: In member function ‘ParameterVirtualWidget* ParameterWidget::createParameterWidget(ParameterObject*, DescriptionStyle)’:
./src/parameter/ParameterWidget.cc:380:1: warning: control reaches end of non-void function [-Wreturn-type]
  380 | }

Edit: This comment was placed on the wrong redlizard PR, and this is actually already merged in master. I pushed a fix for this.

[rcolyer]

@redlizard and I conducted some careful benchmarking on this in the current state. While we discovered there are still some specific scenarios where the GC makes it run slower (up to 20%, but only sometimes) a careful analysis revealed these to be cache related issues. The inclusion of the GC accounting simply causes a little bit more of the cache to be utilized, which we found most discernible during a design with heavy recursion. But depending on system, system load, and other aspects these effects disappear as a measurable difference. And then in the majority of cases and scenarios there is no significant runtime difference. Following this examination I conclude this is a perfectly reasonable performance outcome for this change.