perf(rmw_graph): throttle the stale-slot sweep off the graph query path

[benaliabderrahmane]

query_all ran registry_cleanup_stale before every graph query, and it has 13 call sites, so rmw_get_node_names, rmw_count_publishers, rmw_count_subscribers, rmw_get_topic_names_and_types and the rest all paid for it. The sweep walks every live slot, copies 1164 bytes out of each and stats /proc/ to decide whether the owner is still alive.

Measured on one registry:

live entries    one sweep    via rmw_get_topic_names_and_types
1335            1.4 ms       2.9 ms
9235            21.3 ms      42.6 ms
12671           27.2 ms      54.4 ms

rmw_get_topic_names_and_types calls query_all twice, so it paid twice. Anything polling the graph paid it several times a second whether or not the graph had changed.

Reclaiming slots owned by dead processes is garbage collection and does not belong on a read path. Sweep at most once per second per context instead, guarded by a CAS so concurrent callers skip rather than queue up behind each other.

No guarantee is weakened. A graph query could already return an entity whose owner died just after the sweep that vetted it, so the result was never a liveness statement; this only widens a window that was always open. The full-registry path in registry_add still sweeps unconditionally, since there it is the last resort before entity creation fails.

test_rmw_graph now compiles registry.cpp so it can seed the registry directly.

Closes #52

[Copilot]

Pull request overview

This PR removes the expensive registry_cleanup_stale() sweep from the hot graph-query read path by throttling it (per context) to at most once per second, avoiding an O(live_slots) /proc/<pid> stat pass on every graph query call.

Changes:

• Add per-context stale-cleanup throttling (steady-clock timestamp + CAS) in rmw_graph.cpp.
• Track last cleanup timestamp in UdsContext (last_cleanup_ns).
• Extend graph tests to seed the registry directly (and update test target to compile registry.cpp).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
rmw_unix_socket_cpp/src/rmw_graph.cpp	Introduces maybe_cleanup_stale() throttling logic and routes graph queries through it.
rmw_unix_socket_cpp/src/types.hpp	Adds last_cleanup_ns atomic timestamp to UdsContext to support throttling.
rmw_unix_socket_cpp/test/test_rmw_graph.cpp	Adds a test that validates cleanup is not run on every graph query by injecting “dead” registry entries.
rmw_unix_socket_cpp/CMakeLists.txt	Compiles src/registry.cpp into test_rmw_graph so the test can call internal registry functions.

Suppressed comments (1)

rmw_unix_socket_cpp/test/test_rmw_graph.cpp:95

• This assertion assumes the second graph query happens within the 1s throttle window. On a slow/contended test runner that may not hold, making the test flaky. Consider explicitly refreshing last_cleanup_ns immediately before the second graph query so the test validates the throttle behavior without depending on scheduling/timing.

  const int32_t second = add_ghost("ghost_second");
  ASSERT_GE(second, 0);
  EXPECT_TRUE(graph_lists("ghost_second")) <<
    "cleanup_stale ran again right away; the throttle is not in effect";

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[benaliabderrahmane]

Full adversarial review completed (same process as #60 and #57). Verdict: the core change is sound — the CAS throttle is correctly lock-free, changes only when sweeps run and never what they do, and the "whoever wins sweeps, losers skip" semantics is exactly the pile-up removal intended. No blockers. 11 findings — 3 should-fix, 8 nits — all addressed on this branch.

Should-fix (fixed)

• The throttle test raced the wall clock and asserted a machine-global negative (f42b723). EXPECT_TRUE(graph_lists("ghost_second")) held only if < 1 s elapsed between two queries — a live flake under load or sanitizers. Worse, it asserted a DEAD_PID slot survives in the domain-99 registry, which every fixture-based test binary in the package sweeps unthrottled at rmw_init — so parallel ctest could reclaim the ghost mid-test. The test now runs on a private domain (93), re-arms the throttle by writing last_cleanup_ns stamps deterministically, asserts the sweep resumes after the interval ("once per process lifetime" used to pass identically), and removes its ghosts unconditionally so a failed assertion can't strand slots in the persistent segment.
• rmw_init's sweep was never stamped (1040414). The process's first graph query — moments later, during node discovery, exactly when queries cluster — repeated the identical full stat-every-slot sweep init just paid for (measured 21–27 ms on a large registry). And 0 is a legitimate steady_clock reading near boot, silently inverting "the first query after a quiet period sweeps". One stamp after the init sweep fixes both.
• Crash-detection latency is now documented: the sweep is the only mechanism that reclaims an ungracefully-dead process's slots and produces its doorbell rings, so detection now lags up to one interval per polling context (pre-throttle a 10 Hz poller reclaimed within ~100 ms). This matters more after feat(tl): pull-based TRANSIENT_LOCAL replay; interest-scoped doorbell #60, where the throttled sweep is also the steady-state reclaimer of a dead TRANSIENT_LOCAL publisher's latched-cache segment. Deliberate tradeoff, now stated at maybe_cleanup_stale.

Nits (fixed or recorded)

• Comments said "per process" where the throttle is per context (multiple contexts per process are legal and sweep independently) — reworded.
• The CMake note justified compiling registry.cpp into test_rmw_graph with "the registry keeps no per-process state", which is checkably false (ring_doorbells holds a thread_local send-fd cache); the conclusion held, the premise didn't — reworded to the accurate invariant.
• Recorded, no code change: a CAS loser proceeds straight to registry_query and may observe dead-owner slots the in-flight sweep is about to reclaim — that's the pile-up removal working as intended.

Heads-up for merge ordering

perf/graph-query-generation-cache independently implements the same throttle in the same region of rmw_graph.cpp (its own steady_now_ns, its own maybe_cleanup_stale, its own last_graph_cleanup_ns field). Whichever branch lands second should rebase onto the other's throttle rather than adding a parallel one — two independent timestamps for one sweep would be wrong. If this lands first, that branch should keep only its generation cache.

Verification

Full 14-suite ctest green on this branch, and on integration/tl-pull-launch-fixes (devel + #60 + this PR + #57 with the same fixes).

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (2)

rmw_unix_socket_cpp/test/test_rmw_graph.cpp:22

• The test already adds src as an include directory (see target_include_directories(test_rmw_graph PRIVATE src)), so using ../src/... includes is unnecessarily brittle and can break if the test file moves or the build is out-of-source. Prefer including the internal headers via the configured include path.

#include "../src/registry.hpp"
#include "../src/types.hpp"

rmw_unix_socket_cpp/CMakeLists.txt:205

• test_rmw_graph links against the rmw_unix_socket_cpp shared library (which already includes src/registry.cpp), and also compiles src/registry.cpp into the test executable. This creates two global definitions of rmw_uds::registry_* in the same process and can cause symbol interposition (the shared library may end up calling the test's copy), making tests brittle and behavior dependent on linker/runtime symbol resolution. Prefer calling rmw_uds::registry_add/remove from the shared library directly (include registry.hpp), or factor registry into a reusable OBJECT/static target used by both the shared lib and tests (single definition).

  # test can seed the shared registry directly; the rmw library does not export
  # those internal symbols. Both copies act on the same shared memory; the only
  # process-local state in registry.cpp is ring_doorbells' thread_local send-fd
  # cache, one per copy, so the duplication is benign.
  ament_add_gtest(test_rmw_graph test/test_rmw_graph.cpp src/registry.cpp)