perf(flows): remove redundant event scans in prompt assembly - #6540
Open
dexhunter wants to merge 1 commit into
Open
perf(flows): remove redundant event scans in prompt assembly#6540dexhunter wants to merge 1 commit into
dexhunter wants to merge 1 commit into
Conversation
_get_contents walks every event's parts several times per LLM request. The include/exclude filter alone calls _contains_empty_content and the three internal-event predicates in sequence, and each of those restarts the walk from the first part. get_function_responses() and get_function_calls() rebuild a fresh list on every call and the async rearrangement calls them repeatedly for the same event. None of that work depends on anything that changes between the calls. This removes the repetition without changing what is assembled: - _should_include_event_in_context derives the two facts it needs (whether any part is visible, and whether the event is an internal ADK event) in a single pass, then applies the same boolean expression with the same operand order and short-circuit structure. - _rearrange_events_for_async_function_responses_in_history computes the function-call and function-response lists once per event and reuses them. - The compaction probe folds into an early-exit loop, and the loop-invariant strip_client_function_call_ids branch is hoisted out of the per-part copy. Every helper is kept and the rendered prompt is unchanged. Measured on a 501-event tool-using history (125 turns, function_call and function_response payloads), CPU time for one _get_contents call, baseline and change alternating within one measurement window over 14 paired repetitions: baseline 4.8001 ms change 4.0470 ms paired delta -15.47% median, -22.48% to -9.83%, all 14 repetitions negative Adds regression tests pinning the three filter contracts this refactor has to preserve: internal ADK events stay excluded whatever else the event carries, a compaction event is not empty but is still branch-filtered, and a content with parts but an empty role still counts as empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
No existing issue, so this description follows the issue-template structure.
Problem:
_get_contentsruns once per LLM request over the whole session history, and it walks each event'spartsseveral times.The include/exclude filter shows this most plainly.
_should_include_event_in_contextcalls_contains_empty_content,_is_adk_framework_event,_is_auth_eventand_is_request_confirmation_eventin sequence. Each one restarts the walk from the first part, so a two-part event gets scanned up to five times to answer one question. Separately,get_function_responses()andget_function_calls()rebuild a fresh list by rescanning every part on each call, and_rearrange_events_for_async_function_responses_in_historycalls them repeatedly for the same event. None of that repeated work depends on anything that changes between the calls.The cost lands on every request and scales with conversation length, so it grows quadratically over a long session.
Solution:
Remove the repetition without changing what gets assembled:
_should_include_event_in_contextderives the two facts it needs (whether any part is visible, and whether the event is an internal ADK event) in a single pass over the parts, then applies the same boolean expression with the same operand order and short-circuit structure as before._rearrange_events_for_async_function_responses_in_historycomputes the function-call and function-response lists once per event and reuses them.strip_client_function_call_idsbranch moves out of the per-part copy in_copy_content_for_request.The change keeps every existing helper and every docstring.
_contains_empty_content,_is_part_invisibleand the three internal-event predicates still hold the readable definition of the contract. The rendered prompt does not change.Measurements
Session history of 501 events (125 tool-using turns with
function_call/function_responsepayloads). CPU time (time.process_time) for one_get_contentscall. Each repetition builds fresh fixtures, and the collector runs before each timed call and stays off inside it. I measured baseline and change alternately within one window over 14 paired repetitions, so host drift hits both arms equally. This machine is shared, so the paired delta carries the claim rather than either absolute number:_get_contentscallc12a025)All 14 paired repetitions came out negative. That is −0.75 ms of framework CPU per LLM request at this history length.
What this is and is not: framework CPU per request, not user-visible latency.
_get_contentsis a synchronous function on the async request path, so the honest framing is the throughput and concurrency ceiling rather than single-request wall time. The figure is also specific to a long tool-using history; on a short history the absolute saving is proportionally smaller. I left out the larger item on purpose: roughly 60% of the remaining time is the per-Partand per-Contentshallow copy that gives downstream processors session isolation. Removing that copy is worth more than this whole change, but it means changing the isolation contract400f512destablished, which is your call rather than something to slip into a perf PR.The optimization search that produced the idea is recorded at https://dashboard.weco.ai/share/HBCrQEvgNK-VH8KnBeTtcIV_-h1NFbi0 as a record of the search only. I wrote and reviewed the diff in this PR by hand. I did not use the search's own best candidate, because it dissolved several helpers and diverged from upstream behaviour on the edge cases the tests below now cover.
Testing Plan
Unit Tests:
Added four regression tests pinning the filter contracts this refactor has to preserve. Each one fails against a version of the filter that gets the corresponding edge case wrong:
test_internal_events_excluded_even_with_transcription: an internal ADK event stays excluded whatever else it carries, since the exclusion does not depend on the emptiness check (parametrized overadk_framework,adk_request_credential,adk_request_confirmation).test_internal_event_with_empty_role_and_transcription_excluded: the same, for the combination where a transcription keeps the event non-empty while an empty role keeps the parts from counting as visible.test_compaction_event_included_but_still_branch_filtered: a compaction event is not empty, but branch filtering still applies to it.test_content_with_parts_but_empty_role_is_empty: emptiness depends on the role as well as the parts.Beyond the tests, I compared prompt output byte-for-byte against the unmodified code across randomized histories under four argument combinations (default,
preserve_function_call_ids=True,include_thoughts_from_other_agents=True, and a foreignagent_name), plus a fixture built to reach the awkward shapes: compaction events both plain and simultaneously framework/auth/confirmation, empty-role contents, thought-only parts, transcription-only events, branch scoping and cross-agent replies. The rendered contents are identical in every case.Manual End-to-End (E2E) Tests:
This is a pure refactor of prompt assembly with byte-identical output, so the behavioural evidence is the byte-for-byte comparison above rather than a console transcript. To reproduce the measurement, the repo's own harness covers the same function:
Note that
tests/unittests/cli/utils/test_cli_tools_click.py::test_telemetry_cli_commandsfails onc12a025both with and without this change. It is unrelated and pre-existing.Checklist