Speed up ValidateCreatedForThisSystem() for inner-loop use

[sherm1]

As discussed in PR #15489, ValidateCreatedForThisSystem() wasn't lightweight enough for performance-sensitive uses. This PR fixes that by:

• Removing null pointer checks for non-pointer types,
• Performing a single test to reject either system id mismatches or an object with no system id at all.
• Moving the error-message generation out of the method body to permit inlining.

Also:

• ValidateCreatedForThisSystem() moves to SystemBase to be next to its sister method ValidateContext().
• Uses that caused a redundant null pointer check are fixed.
• Adds a new internal use method to Identifier to facilitate the combined invalid-or-mismatch test.
• Updates ValidateContext() description for completeness and consistency with ValidateCreatedForThisSystem().
• Adds some missing unit tests for both methods.

---

This change is 

[sherm1]

+@jwnimmer-tri for feature review, please

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

[jwnimmer-tri]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @sherm1)

---

systems/framework/system_base.h, line 850 at r1 (raw file):

      }
    }();
    if (id != system_id_)

Per the documentation of Identifier::operator!=, a precondition is that both IDs are already known to be valid. We know through our class invariants that system_id_ is known to be valid, but I don't think we know whether or not object.get_system_id() has already been checked for validity? The prior implementation of this function on System had a check for is_valid() on the argument, and we've lost that here in SystemBase. I think we need to put it back? I seem to recall that when we tried to prove that an is_valid check was redundant during the PR train for all the possible Clazz types, the call sequences were so deep that we gave up and decided to guard here.

If the extra is_valid check (zero-check) is showing up in profiling, then as one possible solution, perhaps we want a new "NaN-enabled" comparison function to be added to Identifier, where it just compares the underlying ints, without any preconditions? Basically it would be sugar for (a.is_valid() && b.is_valid() && (a == b)) || (!a.is_valid() && !b.is_valid()) which is a valid call sequence, just one that we want to perform more quickly during an inner loop here.

---

systems/framework/system_base.cc, line 284 at r1 (raw file):

}

void SystemBase::ThrowNotCreatedForThisSystemImpl(

The "... lacks a system_id ..." message from the original System implementation of this check seems like a relatively important diagnostic that disambiguates two different error conditions, and seems easy to replicate even in the SystemBase cc file, either by passing the SystemId into this function, or by passing bool valid instead.

---

systems/framework/system_output.h, line 82 at r1 (raw file):

  }

  /** (Internal) Gets the id of the System that created this output.

FYI to other reviewers... this escalation to public brings this class up to par with all of the other framework data types (InputPort, OutputPort, Parameters, DiscreteValues, etc.), and is also required in order for SystemBase to have access to it, since only System<T> is friended.

[sherm1]

With a little more yak-shaving now I'm very happy with ValidateCreatedForThisSystem(). PTAL.
+@sammy-tri for platform review per rotation, please

Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform) (waiting on @jwnimmer-tri and @sammy-tri)

---

systems/framework/system_base.h, line 850 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Per the documentation of Identifier::operator!=, a precondition is that both IDs are already known to be valid. We know through our class invariants that system_id_ is known to be valid, but I don't think we know whether or not object.get_system_id() has already been checked for validity? The prior implementation of this function on System had a check for is_valid() on the argument, and we've lost that here in SystemBase. I think we need to put it back? I seem to recall that when we tried to prove that an is_valid check was redundant during the PR train for all the possible Clazz types, the call sequences were so deep that we gave up and decided to guard here.

If the extra is_valid check (zero-check) is showing up in profiling, then as one possible solution, perhaps we want a new "NaN-enabled" comparison function to be added to Identifier, where it just compares the underlying ints, without any preconditions? Basically it would be sugar for (a.is_valid() && b.is_valid() && (a == b)) || (!a.is_valid() && !b.is_valid()) which is a valid call sequence, just one that we want to perform more quickly during an inner loop here.

Done, PTAL.

---

systems/framework/system_base.cc, line 284 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The "... lacks a system_id ..." message from the original System implementation of this check seems like a relatively important diagnostic that disambiguates two different error conditions, and seems easy to replicate even in the SystemBase cc file, either by passing the SystemId into this function, or by passing bool valid instead.

Done.

[jwnimmer-tri]

still.

Could we do -@sammy-tri +@rpoyner-tri for platform review, as subject-matter expert?

Reviewed 4 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform) (waiting on @rpoyner-tri and @sherm1)

---

systems/framework/system_base.h, line 830 at r2 (raw file):

  @note This method is sufficiently fast for performance sensitive code.
  @throws std::exception if the System Ids don't match.
  @pre both `context` and `this` have valid System Ids. */

I think this the new @pre documentation is helpful for those of us who maintain these implementation, but should not be exposed to users.

Writing down representation invariants as @pre doxygen on public seems quite likely to confuse our users. Doubly so in this case, since the user has no ability to write any code themselves to confirm or deny the invariant -- the can never touch an ID directly, since it is an internal class; the only thing they are allowed to do related to IDs is to call one of the Validate... functions. As such, Validate should have a total contract, without preconditions.

While it is the case that the condition is assumed by this implementation, and that fact is of interest to the Drake developers who main these functions, it's not incumbent upon the caller of this function to guarantee the condition; it's incumbent on the framework's private implementation details to guarantee it. As such, it could appear (as most) as documentation in the private section near the member fields, for example, or comments within the body of this function implementation, but should not leak into the public documentation for users.

Ditto throughout for the all of the new @pre text.

---

systems/framework/system_base.cc, line 286 at r3 (raw file):

void SystemBase::ThrowNotCreatedForThisSystemImpl(
    const std::string& nice_type_name, internal::SystemId id) const {
  if (!id.is_valid())

nit GSG requires {} on multi-line if-else bodies.

[rpoyner-tri]

Reviewed 3 of 7 files at r1, 2 of 4 files at r2.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri and @sherm1)

[sherm1]

All review comments addressed.
Thanks, @jwnimmer-tri & @rpoyner-tri!

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

systems/framework/system_base.h, line 830 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this the new @pre documentation is helpful for those of us who maintain these implementation, but should not be exposed to users.

Writing down representation invariants as @pre doxygen on public seems quite likely to confuse our users. Doubly so in this case, since the user has no ability to write any code themselves to confirm or deny the invariant -- the can never touch an ID directly, since it is an internal class; the only thing they are allowed to do related to IDs is to call one of the Validate... functions. As such, Validate should have a total contract, without preconditions.

While it is the case that the condition is assumed by this implementation, and that fact is of interest to the Drake developers who main these functions, it's not incumbent upon the caller of this function to guarantee the condition; it's incumbent on the framework's private implementation details to guarantee it. As such, it could appear (as most) as documentation in the private section near the member fields, for example, or comments within the body of this function implementation, but should not leak into the public documentation for users.

Ditto throughout for the all of the new @pre text.

Done.

---

systems/framework/system_base.cc, line 286 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit GSG requires {} on multi-line if-else bodies.

Done.

[jwnimmer-tri]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @sherm1)

[rpoyner-tri]

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @sherm1)

[rpoyner-tri]

Reviewed 2 of 7 files at r1, 1 of 3 files at r3, 1 of 2 files at r4.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @sherm1)