Should EvalOutput return a reference instead of expecting an actual vector where to write?

[amcastro-tri]

Consider a system with two output ports:

1. Output one is the state of the system.
2. Output two is a cached entry (assume we have cache, since we will) computation.

When evaluating output 1 we'd need to return a (const) reference to the state vector and avoid unnecessary copies. When evaluating output 2 we'd check for the validity of the cache entry. If valid, we just return the stored value. If not, we update the value. Then return a (const) reference to the now for sure updated cache entry.

This happens now for RBP where we have an output for the state vector and (we will have) an output for poses. Similar cases are the integrator (outputs state), pass through (outputs a reference to the input).

However our System<T>::EvalOutput mechanism does not return references to the appropriate entries in the context but expects the user to write on SystemOutput<T>. In addition, the method AllocateOutput<T> allocates output vectors for all ports even if not needed. Like in the (very frequent) case of the state being an output, we wouldn't need to allocate an output vector but just reuse the state vector.

Would an API like AbstractValue& EvalOutputPort(Context* context, int port_number) returning a reference to the updated value of the output port be an option? @david-german-tri, what do you think? I wonder if this goes along any of the issues you opened and if so sorry for duplicating. I just had this doubt when implementing RBP stuff.

[sherm1]

AbstractValue& EvalOutputPort(Context* context, int port_number)

A further improvement could be:

const AbstractValue& value = output_port.Eval(context);

because that would permit dispatching directly to the evaluator for a particular output port without having to switch or index off the port_number.

[david-german-tri]

This is closely related to #3155, and a little bit related to #2890. It seems to me there are a continuum of options here:

Status quo: Output data must be copied into buffers that the simulator or parent context owns.

3155 proposal: Output data is an abstraction is owned by the simulator or parent context. This abstraction may be satisfied either by an owned buffer or by a reference to data owned by the system context (state or cache).

3482 proposal: Output data must be a reference to data owned by the system context.

I have to say that I prefer #3155 to #3482, because it's just as efficient and allows a graceful transition from the status quo.

However, the more I think about allowing outputs to be references, the more dangerous it seems to me. If a system's outputs can change without calling EvalOutput at all, all sorts of havoc becomes possible. System authors, especially discrete system authors, will have to be extremely careful not to change the cache data that backs an output as a side effect of some other computation.

Maybe the framework can impose the right discipline on a reference-based system somehow. More thought needed. However, I really like that correctness is easy to prove in the status quo: systems cannot mess with their outputs outside of EvalOutput, because they do not have access to that memory, full stop.

[jwnimmer-tri]

I, for one, am happy to defer zero-copy optimizations until we have benchmarks up and running. If people are really concerned about these performance questions, then prioritize writing the benchmarks.

[liangfok]

Should we institute the following rule?

No performance optimization may be merged into master without a corresponding benchmark that proves its efficacy?

[sherm1]

I think the best way to resolve this is with uniform treatment of computations performed in the System framework rather than treating output port values as a special case -- that makes it unnecessarily difficult to reason about. An output port value should resolve to one of the five existing value sources in the Context: time, an input port, a state variable, a parameter, or a cache entry (I mean "state" in all its various forms including modes and samples). Each of those value sources maintains a list of downstream dependents ("value listeners") that need to be invalidated when the value source changes. Invalidation should be propagated through the output port so I don't see how that creates a problem; EvalOutputPort() is not necessary for correctness, just to trigger computation. OTOH, if an output port's value purports to be a state vector (which is always valid) adding a buffer in between introduces the possibility that it could be out of date; that is a real problem.

In this view a subsystem's output port is just a carefully-controlled window into a value in that subsystem's subcontext. The semantics are then just inherited from those of the value source.

To be concrete, I propose we

• get rid of the separately-maintained output value and use the Context instead
• have EvalOutputPort() return a reference to an appropriate context value
• ensure that invalidation is appropriately propagated through output ports

[sherm1]

Should we institute the following rule?

No. We often build on past work. For example, using a map or heap sort is an optimization based on decades of past research in CS; we don't need to rejustify those ourselves. Subfields like optimization, linear algebra, and multibody dynamics have their own rich histories and we should employ the appropriate techniques. For example, numerical linear algebra is mostly concerned with avoiding unnecessary copying, based on extensive performance measurement. The problems are unnoticeable for small systems but have significant architectural impact. System 2 is a framework for numerical computation and we should architect it now to avoid unnecessary copies; that isn't difficult. In our case this also has semantic implications because injecting extra copies introduces the possibility that the copy will be out of date with the original.

[liangfok]

Just wondering: Will getting rid of the separately-maintained output value and using the Context to store this value render output ports to be just "for show"?

[david-german-tri]

It's my own fault for broadening the scope, but this discussion no longer fits in a GitHub issue. It should be discussed f2f eventually. I'm in no hurry to do that, but if someone else feels this is important enough to drive forward now, I'd of course want to participate.

[sherm1]

Will getting rid of the separately-maintained output value and using the Context to store this value render output ports to be just "for show"?

Not at all. The output ports would still be there providing exactly the same useful abstractions; which piece of memory holds the value internally is an implementation issue.

[jwnimmer-tri]

Issues should have owners. Tagging david-german, but feel free to kick back to amcastro if you wish.

[sherm1]

This issue got addressed along the way so is obsolete now. Output ports have an Eval() method that returns a reference to an object that resides in the Context, generally a cache entry value.