Skip to content

perf(runtime): clone only the selected instance during dispatch - #14545

Closed
jthomson04 wants to merge 2 commits into
mainfrom
jthomson04/avoid-instance-list-clone
Closed

jthomson04 wants to merge 2 commits into
mainfrom
jthomson04/avoid-instance-list-clone

Conversation

@jthomson04

@jthomson04 jthomson04 commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Overview:

Summary

Avoid cloning the complete discovered-instance list when resolving a worker's request transport. Clone only the selected instance.

Details:

  • Add a crate-private Client::instance_by_id helper that searches the current discovery snapshot and returns an owned instance.
  • Use it for primary and fallback transport lookups. Release the discovery read guard before dispatch and retain the required address copy.
  • Preserve the public instances() API, fallback policy, transport metadata, and error behavior. Each lookup still reads a fresh snapshot.
  • Keep the linear search; add no index, cache, lock, or shared state. A successful lookup clones one instance instead of the complete list plus the selected instance. A missing lookup clones none.

Where should the reviewer start?

Client::instance_by_id in lib/runtime/src/component/client.rs, then its use in PushRouter::resolve_transport in lib/runtime/src/pipeline/network/egress/push_router.rs.

Validation

Initial validation on fa68d17ced:

  • Client test module: 22 passed, including the lookup test for successful/missing IDs, updated metadata, removal, and retained instances.
  • Push-router test module: 39 passed, including fallback restrictions and missing-worker cases.
  • cargo check --locked -p dynamo-runtime passed.
  • cargo clippy --locked -p dynamo-runtime --lib -- -D warnings passed.
  • Formatting checks for both changed files and git diff --check passed.

Review cleanup on 37bf2ae47f: the focused lookup test, changed-file formatting check, and git diff --check passed. The follow-up removes only two redundant assertions and one doc comment; production logic is unchanged.

No new benchmark or profile was run; this PR makes no measured throughput claim.

Related Issues

  • Confirmed — no related issue

Summary by CodeRabbit

  • Bug Fixes
    • Improved instance lookup during network routing.
    • Preserved existing transport address, type, and fallback behavior when resolving destinations.
    • Ensured routing reflects current instance discovery updates and removals.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 requested a review from a team as a code owner September 9, 2026 17:35
@github-actions github-actions Bot added the perf label Sep 9, 2026
@coderabbitai

coderabbitai Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c20ac9d3-aad5-420c-8258-63b807b8503a

📥 Commits

Reviewing files that changed from the base of the PR and between 8d81d9e and fa68d17.

📒 Files selected for processing (2)
  • lib/runtime/src/component/client.rs
  • lib/runtime/src/pipeline/network/egress/push_router.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

Changes

Instance transport resolution

Layer / File(s) Summary
Snapshot instance lookup and validation
lib/runtime/src/component/client.rs
Client::instance_by_id returns an owned instance from the current discovery snapshot. Tests cover successful lookup, missing IDs, ownership, updates, and removals.
Push router transport resolution
lib/runtime/src/pipeline/network/egress/push_router.rs
resolve_transport uses instance_by_id while preserving TCP/NATS mapping and fallback behavior.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to fa68d

Transport resolution now retains only the selected discovered instance before dispatch, reducing unnecessary cloning while preserving existing lookup, fallback, and transport behavior. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly describes the main performance change: cloning only the selected instance during dispatch.
Description check ✅ Passed The description covers the overview, implementation details, reviewer starting points, validation results, and required related-issues confirmation. It matches the pull request objectives.
  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

Comment thread lib/runtime/src/component/client.rs Outdated
Comment thread lib/runtime/src/component/client.rs Outdated
Comment thread lib/runtime/src/component/client.rs Outdated
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
assert_eq!(client.instance_by_id(1), Some(instances[0].clone()));
let selected = client.instance_by_id(2).unwrap();
assert_eq!(selected, instances[1]);
assert!(client.instance_by_id(3).is_none());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The never-present ID check is an extra missing-ID parameter that protects no distinct observable contract. instance_by_id only searches the current watch snapshot, so the later client.instance_by_id(2).is_none() after sending a snapshot without id 2 exercises the same no-match behavior while also preserving the current-snapshot removal regression; the id 2 equality assertion still protects selecting the requested non-first instance.

🤖 AI Fix

Remove the pre-update client.instance_by_id(3).is_none() assertion and keep the post-removal missing-ID assertion.

jthomson04 added a commit that referenced this pull request Sep 23, 2026
Incorporate the final change from PR #14545. Borrow the live discovery snapshot and clone only the selected instance, preserving transport fallback behavior and the existing lookup regression test.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
jthomson04 added a commit that referenced this pull request Sep 29, 2026
Incorporate the final change from PR #14545. Borrow the live discovery snapshot and clone only the selected instance, preserving transport fallback behavior and the existing lookup regression test.

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #14666, which merged into main on September 30, 2026 (99ae6e5). Every added and removed line from this PR is included in that merged patch: Client::instance_by_id(), its use in PushRouter::resolve_transport(), and the lookup test. Verified that these changes remain in main at 006e5f3.

@jthomson04 jthomson04 closed this Oct 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant