Perf: avoid repeated orchestration-history scans for tracing - #788
Perf: avoid repeated orchestration-history scans for tracing#788Bernd Verst (berndverst) wants to merge 3 commits into
Conversation
OnRunOrchestratorAsync previously scanned orchestration history to correlate tracing events unconditionally, and for every qualifying new event it rescanned the full past-events list to find the originating TaskScheduled/SubOrchestrationInstanceCreated event - O(new events x past events) work, even when no tracing listener was registered. - Add TraceHelper.HasListeners, a cheap check backed by ActivitySource.HasListeners(), and gate all tracing-correlation work in OnRunOrchestratorAsync behind it so nothing is scanned when no listener is registered. - Replace the LINQ Concat-based scan for the ExecutionStarted event with a single-pass local function. - Add TraceHistoryEventLookup, which builds one dictionary index per work item (built lazily, once) for O(1) repeated lookups instead of rescanning PastEvents per new event, while preserving the exact original first-wins (SubOrchestrationInstanceCreated) and last-wins (TaskScheduled) semantics for duplicate event IDs. No public API changes; trace output, error handling, and replay determinism are unaffected. Adds regression tests for the no-listener fast path, first/last-wins correlation semantics end-to-end, and direct unit tests for TraceHistoryEventLookup. Fixes #769 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9c538f3e-308d-48a3-af7f-b3baf90e97b2
There was a problem hiding this comment.
Pull request overview
Improves the gRPC worker’s tracing correlation performance by avoiding unnecessary orchestration-history scans when tracing is disabled and by indexing past history events for O(1) correlation lookups when tracing is enabled. This aligns with the SDK’s goal of keeping worker-side orchestration processing efficient, especially for long-running instances with large histories.
Changes:
- Added a cheap listener check (
TraceHelper.HasListeners) and gated trace-correlation work inOnRunOrchestratorAsyncbehind it. - Replaced repeated past-history scans with a per-work-item indexed lookup (
TraceHistoryEventLookup) that preserves prior first/last-wins semantics for duplicate event IDs. - Added unit and worker-level tests covering listener/no-listener behavior and duplicate-ID correlation semantics.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/Worker/Grpc.Tests/TraceHistoryEventLookupTests.cs | Adds focused unit tests for the new indexed lookup, including duplicate-ID first/last-wins semantics and filtering by event type. |
| test/Worker/Grpc.Tests/GrpcDurableTaskWorkerTests.cs | Adds end-to-end worker tests for the no-listener fast path and for correlation semantics under a real ActivityListener. |
| src/Worker/Grpc/GrpcDurableTaskWorker.Processor.cs | Gates tracing correlation behind HasListeners, replaces LINQ concat scan with a single-pass helper, and uses the indexed lookup for repeated correlation. |
| src/Shared/Grpc/Tracing/TraceHistoryEventLookup.cs | Introduces a lazily-built, cached dictionary index for past events to avoid O(N×M) repeated scans. |
| src/Shared/Grpc/Tracing/TraceHelper.cs | Exposes HasListeners backed by ActivitySource.HasListeners() to allow callers to skip trace-correlation work when tracing is disabled. |
…history-scan-perf
Quantitative performance impactI compared live head Let
Representative, best, and adverse cases“Representative” below means a selected ordinary shape, not production telemetry.
The no-listener ~6 ns figures are close to benchmark-harness cost and should be read as “the scan was removed,” not as a credible millions-of-operations throughput claim. Memory and GCThe lazy indexes reference existing events; they do not copy protobuf messages. On this x64 runtime, estimated incremental dictionary storage while live is approximately 28-56 B per unique indexed ID, including capacity slack/array headers but excluding the existing event objects.
Thus:
Whole-worker throughput interpretationCorrelation-stage speedup does not translate directly to orchestration throughput. Using Amdahl's law:
Those are sensitivity examples. Actual gains require telemetry for listener prevalence and the distributions of |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 01dc5b7c-c742-45b7-ae8c-5cda9bcfbd2c
| /// events (e.g. "TaskCompleted") back to the history event that scheduled them (e.g. "TaskScheduled"). | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// The indexes are built lazily, at most once per instance, and cached for the lifetime of the instance. This |
There was a problem hiding this comment.
Can we modify this comment? It's misleading because the index is not cached for the lifetime of the instance, but rather just for the lifetime of the workitem for the instance. The instance could have multiple work items, in which case we recreate this map, right?
So maybe just changing to "cached for the lifetime of the instance work item" would be more accurate
| /// <param name="eventId">The event ID to look up.</param> | ||
| /// <returns>The matching event, or <see langword="null"/> if none is found.</returns> | ||
| /// <remarks> | ||
| /// If more than one "TaskScheduled" event shares the given event ID, the last one encountered (in history |
There was a problem hiding this comment.
We should actually throw an exception in this case, this situation should be impossible. It should be 1:1
| /// <param name="eventId">The event ID to look up.</param> | ||
| /// <returns>The matching event, or <see langword="null"/> if none is found.</returns> | ||
| /// <remarks> | ||
| /// If more than one "SubOrchestrationInstanceCreated" event shares the given event ID, the first one |
| } | ||
|
|
||
| [Fact] | ||
| public async Task DispatchWorkItem_OrchestratorRequest_WithActivityListener_UsesFirstAndLastWinsSemantics() |
There was a problem hiding this comment.
As per the comment in TraceHistoryEventLookup, we actually want to throw an exceptoin in this case, so let's update the test appropriately
| // TraceHelper, so they are kept in this class (whose test methods xunit runs sequentially by default) to | ||
| // avoid flaky interference between them. | ||
| [Fact] | ||
| public async Task DispatchWorkItem_OrchestratorRequest_NoActivityListeners_SkipsTracingWorkAndCompletes() |
There was a problem hiding this comment.
Can we confirm that the TraceHistoryEventLookup methods are not invoked in this case?
There was a problem hiding this comment.
Can we add any integration tests for this?
| public class TraceHistoryEventLookupTests | ||
| { | ||
| [Fact] | ||
| public void GetTaskScheduledEvent_DuplicateEventIds_ReturnsLastMatch() |
There was a problem hiding this comment.
Same comment about new semantics
| } | ||
|
|
||
| [Fact] | ||
| public void GetSubOrchestrationInstanceCreatedEvent_DuplicateEventIds_ReturnsFirstMatch() |
Summary
OnRunOrchestratorAsyncinsrc/Worker/Grpc/GrpcDurableTaskWorker.Processor.csscanned orchestration history to correlate tracing events unconditionally, and rescanned the full past-events list for every qualifying new event to find the originatingTaskScheduled/SubOrchestrationInstanceCreatedevent — O(new events × past events), even when no tracing listener was registered.This is a precise, internal-only performance fix:
TraceHelper.HasListeners, a cheap check backed byActivitySource.HasListeners(), and gated all tracing-correlation work inOnRunOrchestratorAsyncbehind it, so nothing is scanned when no listener is registered.Concat-based scan for theExecutionStartedevent with a single-pass local function.TraceHistoryEventLookup(src/Shared/Grpc/Tracing/TraceHistoryEventLookup.cs), which builds one dictionary index per work item (lazily, at most once per event type) for O(1) repeated lookups instead of rescanningPastEventsper new event, while preserving the exact original first-wins (SubOrchestrationInstanceCreated) and last-wins (TaskScheduled) semantics for duplicate event IDs.No public API changes. Trace output, error handling, and orchestration replay determinism are unaffected — this only changes how existing history is looked up, not what is looked up or emitted.
Tests
test/Worker/Grpc.Tests/TraceHistoryEventLookupTests.cs(new): direct unit tests forTraceHistoryEventLookupcovering duplicate-event-ID first/last-wins semantics, no-match, event-type filtering, and empty input.test/Worker/Grpc.Tests/GrpcDurableTaskWorkerTests.cs(added):ActivityListenerand asserts the resulting activities reflect the original first/last-wins correlation semantics.Worker.Grpc.Testssuite passes (145/145).Worker.Grpcbuilds cleanly across all target frameworks (netstandard2.0;net6.0;net8.0;net10.0).Fixes #769