Planner comparison and update

[milroy]

Edit: this PR has grown in scope to enable updates to planner and planner_multi

To update the Fluxion resource graph, its traverser will need to be reinitialized. The initialization process assembles vectors of resource counts and types via graph traversal to update the planner_multi pruning filters. Currently, counts and types are stored as separate vectors in planner_multi under the assumption that their order and size don't change. The index of a count must be the same as the index of the corresponding resource type string. This assumption is valid as long as the resource graph is static, but allowing the resource graph to change without updating the planners and their metadata will cause scheduling inconsistency (e.g., incorrect traversal pruning) because of incorrect resource types and counts.

This PR adds comparison operators for fundamental planner member data classes to scope the comparison to the correct part of the code. The rescoping avoids relying on awkward class functions for comparison of member data objects and their data.

The PR also restructures planner_multi to use boost::multi_index container to replace and simplify planner_multi member data. The boost::multi_index container has the important capability of searching and comparing by multiple indices which can be vector-like (random_access), and hash-like (hashed_unique). Adding two indexes for lookups enables support for altering resource types, counts, and their orders (indices) in a natural way. Performing these transformations with vectors would be higher complexity both in time and lines of code.

Finally, the PR enables the update functionality in the planner and planner_multi C interfaces and creates unit tests for updating planner and planner_multi.

Edit: this PR also re-enables the Valgrind tests and fixes UB in planner unit tests.

[jameshcorbett]

Were there more changes you wanted to add @milroy ? It looks reasonable to me, although the commits need bodies.

Question though--is the motivation for this mostly to make the code more idiomatic? It doesn't look like it saves much complexity.

[milroy]

Deleting a question I asked above because there's a pretty obvious answer that just took a bit of searching.

[trws]

Guessing the answer was UB propagation from the undefined value of bo? I started to type an answer earlier but didn’t get it done before we had to board. Starting a review.

[trws]

Pending the value safety question this looks great.

[trws]

Is m_total_resources guaranteed to be smaller than resource_total?

[milroy]

The case where resource_total > m_total_resources is when the graph is being grown, and resource_total < m_total_resources means the graph is being shrunk. There aren't any guarantees on the value of resource_total except that it's nonnegative.

@jameshcorbett asked a form of this question, too, and my reasoning has been that negative values for point->remaining (e.g., when (point->remaining + delta) < 0) shouldn't cause problems because they should only happen for reservations (which will be cleared at the next schedule loop). I was thinking that adding a check for negative remaining wasn't worth the performance penalty.

That said, I don't expect update_total to be called often, and adding a guard check may prevent complicated problems with removing spans if that process isn't carefully performed upstream (i.e., in planner_c_interface).

[milroy]

Guessing the answer was UB propagation from the undefined value of bo?

Yeah, that was it, and I added two commits to fix other occurrences in the unit tests. I thought I was right about the comma-separated syntax for declaration and initialization and went down the wrong investigative hole for a while.

before we had to board

Thanks for the review and have a good flight!

[milroy]

Thanks for the reviews all! Setting MWP.

[codecov]

Codecov Report

Attention: Patch coverage is 85.37736% with 31 lines in your changes missing coverage. Please review.

Project coverage is 70.9%. Comparing base (cd2fb0f) to head (6f16387).
Report is 308 commits behind head on master.

Files with missing lines	Patch %	Lines
resource/planner/c++/planner_multi.cpp	83.5%	16 Missing ⚠️
resource/planner/c/planner_multi_c_interface.cpp	78.3%	13 Missing ⚠️
resource/planner/c++/planner.cpp	93.1%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1061     +/-   ##
========================================
+ Coverage    70.6%   70.9%   +0.2%     
========================================
  Files          95      96      +1     
  Lines       12744   12819     +75     
========================================
+ Hits         9010    9099     +89     
+ Misses       3734    3720     -14     

Files with missing lines	Coverage Δ
resource/planner/c++/mintime_resource_tree.cpp	90.4% <100.0%> (+0.3%)	⬆️
resource/planner/c++/mintime_resource_tree.hpp	100.0% <ø> (ø)
resource/planner/c++/planner_internal_tree.cpp	100.0% <100.0%> (ø)
resource/planner/c++/planner_internal_tree.hpp	100.0% <ø> (ø)
resource/planner/c++/planner_multi.hpp	100.0% <100.0%> (ø)
resource/planner/c++/scheduled_point_tree.cpp	92.3% <100.0%> (+11.2%)	⬆️
resource/planner/c++/scheduled_point_tree.hpp	100.0% <ø> (ø)
resource/planner/c/planner_c_interface.cpp	70.1% <100.0%> (+0.1%)	⬆️
resource/traversers/dfu_impl.hpp	94.7% <100.0%> (+0.1%)	⬆️
resource/planner/c++/planner.cpp	83.5% <93.1%> (+0.8%)	⬆️
... and 2 more