[systems] Add diagram life support

[rpoyner-tri]

This patch adds an internal properties interface that allows arbitrary objects to track the lifetime of systems that get transferred from a diagram builder to a diagram.

---

This change is 

[rpoyner-tri]

Progress toward mortal diagrams: #14387 .
+(release notes: none)
This is probably one of the more contentious pieces, so +@jwnimmer-tri for feature review (despite the wait).

I'll update the prototype at #22022 shortly, to give this more context.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

a discussion (no related file):
In the current draft of #22022, this patch is critical to the functioning of RobotDiagramBuilder(RDB)/RobotDiagram(RD). Python programs add systems via RDB::builder().AddSystem() or similar, creating cycles between each system and the contained builder. The RDB::Build() call transfers the entire builder under the covers (via private/friend RD ctor), which calls builder.BuildInto(). Thanks to the internal properties mechanism, a ref to the Python wrapper of RDB::builder() gets passed along the whole chain.

A consequence of this is that client programs actually have to maintain a ref to RD; they can't just, say, keep a plant ref and expect magic to happen.

---

[jwnimmer-tri]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

systems/framework/diagram.h line 66 at r1 (raw file):

Duplicated here from diagram_builder.h to avoid a header dependency cycle.

We already have the dependency that diagram_builder.h depends on diagram.h (e.g., for the blueprint class). The diagram_builder.h can rely on this definition in diagram.h, without any need for copy-paste.

---

systems/framework/diagram.cc line 1435 at r1 (raw file):

  // TODO(rpoyner-tri): consider what to do with diagram_properties under
  // scalar conversion. For now, do nothing.

I do not buy deferring this question with a TODO. We should decide what the correct answer is and do that immediately in this PR. We can always change our mind later and revise the decision.

---

systems/framework/diagram_builder.h line 40 at r1 (raw file):

template <typename T>
DiagramProperties&
get_mutable_properties(DiagramBuilder<T>* builder) {

BTW If DiagramProperties were a struct instead of a typedef, then we could avoid these friend hijinks. We could have a public-access get_mutable..() method on DiagramBuilder, and since it's return value (struct) was in the namespace internal, it would naturally be forbidden / hidden from users.

---

systems/framework/diagram.h line 65 at r1 (raw file):

};

// A type for arbitrary properties that can be attached to a diagram.

This overview comment is so non-specific as to be nearly meaningless and unhelpful. We should have a longer overview comment that fully explains the situation -- the pydrake FFI needs to track / launder lifetime details through the DiagramBuilder => Diagram evolution, and this is the meeting point between Python and C++ to accomplish that.

---

systems/framework/diagram.h line 67 at r1 (raw file):

// A type for arbitrary properties that can be attached to a diagram.
// Duplicated here from diagram_builder.h to avoid a header dependency cycle.
using DiagramProperties = string_map<std::any>;

Why std::any instead of our existing technologies shared_ptr<const void> or AbstractValue?

I suppose the goal is reference-counting semantics, in which case the value semantics of AbstractValue would not be correct.

That still leaves shared_ptr<const void> as a contender. If we ever need to extract (downcast) the held object in production code, then that would be a distinguishing factor for shared_ptr vs any, but from what I could see in the WIP branch, we only ever hold the object, we never downcast it to operate on.

---

systems/framework/diagram.h line 67 at r1 (raw file):

// A type for arbitrary properties that can be attached to a diagram.
// Duplicated here from diagram_builder.h to avoid a header dependency cycle.
using DiagramProperties = string_map<std::any>;

BTW I don't love DiagramProperties as a name -- it doesn't really communicate the behaviors and architecture we are aiming for / using here.

These properties are not really a systems framework thing, and really they are not arbitrary / generic properties. They are object lifetime support infrastructure.

Possibly candidates:

• DiagramKeepAlive
• DiagramDependentStorage
• DiagramOrnaments

---

systems/framework/diagram.h line 600 at r1 (raw file):

  internal::OwnedSystems<T> registered_systems_;

  internal::DiagramProperties diagram_properties_;

nit This new field is interrupting the organization of existing member fields. The list of owned systems (prior field) and their index reverse-lookup (next field) are split in twain, yet have strong invariants across them. This new field should sit either right before or right after the event_times_buffer_cache_index_.

[jwnimmer-tri]

modulo clarity fixes.

Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers

[jwnimmer-tri]

Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers

---

systems/framework/diagram.cc line 1435 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I do not buy deferring this question with a TODO. We should decide what the correct answer is and do that immediately in this PR. We can always change our mind later and revise the decision.

(BTW There should still be a comment saying that we are ignoring the field on purpose here.)

[rpoyner-tri]

Reviewable status: 8 unresolved discussions, needs at least two assigned reviewers

---

systems/framework/diagram.h line 67 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why std::any instead of our existing technologies shared_ptr<const void> or AbstractValue?

I suppose the goal is reference-counting semantics, in which case the value semantics of AbstractValue would not be correct.

That still leaves shared_ptr<const void> as a contender. If we ever need to extract (downcast) the held object in production code, then that would be a distinguishing factor for shared_ptr vs any, but from what I could see in the WIP branch, we only ever hold the object, we never downcast it to operate on.

Only value semantics (copyable or cloneable) are needed so far, so the shared_ptr route feels extra. In the prototype, we are capturing a pybind11::object, which does ref-counting on the referred-to python object, which is what we want.

I'll have a look at AbstractValue.

---

a discussion (no related file):
Some notes, since my fevered brain has had time afk to ponder this.

• The solution presented here and in Python gc fixes prototype #22022 so far has the property that the C++/Python DiagramBuilder pair are temporarily immortal, between any AddSystem() -like call and Build(). Since this is still strictly better than master, I don't think it's a showstopper, but probably should be documented somewhere.
• Also, the draft solution only uses the current patch's feature for Python calls DB::AddSystem. If we believe this scheme is necessary, the patch needs to be better organized for wider use in any of the AddToBuilder(), ApplyBlahConfig() methods that do AddSystem under the hood.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers

---

systems/framework/diagram.h line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Only value semantics (copyable or cloneable) are needed so far, so the shared_ptr route feels extra. In the prototype, we are capturing a pybind11::object, which does ref-counting on the referred-to python object, which is what we want.

I'll have a look at AbstractValue.

Hm. AbstractValue is, of course, abstract, so we can't use it as the value_type directly. So then we turn to a pointer type -- shared_ptr<AbstractValue>?

Honestly, I don't know what the objection to std::any is.

[rpoyner-tri]

+@xuchenhan-tri for platform review, by schedule please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)

[xuchenhan-tri]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions

---

a discussion (no related file):
BTW, I'm not seeing the full picture and I don't understand exactly how the DiagramLifeSupport struct help solve the problem. Consequently, I don't see why a string_map is needed in the life support. But I suppose that'll become more clear in future PRs.

---

systems/framework/diagram.h line 65 at r2 (raw file):

};

/// External life support data for the diagram. The data will be moved to the

nit, Use "//" over "///" in internal namespace.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion

---

a discussion (no related file):

Previously, xuchenhan-tri wrote…

BTW, I'm not seeing the full picture and I don't understand exactly how the DiagramLifeSupport struct help solve the problem. Consequently, I don't see why a string_map is needed in the life support. But I suppose that'll become more clear in future PRs.

It might prove to be overkill, but I don't think the cost is that great either.