Hi all,
I'd like to restart the discussion on performance testing. This time
it's about a layer that AIP-59 doesn't cover: micro-benchmarks for
single hot-path methods, run automatically on pull requests.
Why now?
While addressing review comments on Task Iteration (#62922 [1]), a fix
to CommsDecoder.asend changed it from two thread hops per request to
one. Every async SDK call a task makes goes through it: Variables,
Connections, XComs, the task state store. Measured locally (median of
three runs, before -> after):
send, one sync round trip (unchanged) 11.5 us -> 11.4 us (none)
asend, one async round trip 108.7 us -> 58.0 us (-47%)
asend, 50 concurrent coroutines on a loop 18.6 ms -> 11.7 ms (-37%)
The reverse change, an extra thread hop added in passing, would have
cost the same amount, and nothing in CI would have noticed. Today we
only find such regressions when someone measures by hand (or by agent),
often after a release: #71721 [2] reports structured task logging in 3.2.0
multiplying high-volume task runtime by 2.64.
Micro-benchmarks could relate to AIP-59 (Performance tests framework [3]),
but would sit one layer below it, not replace it. They target single methods on
hot paths and flag a regression in the PR that introduces it, in seconds and
on every PR. AIP-59 answers the question micro-benchmarks can't: how a
whole release behaves under load.
Draft PR #74562 [8] adds:
*
task-sdk/benchmarks/, next to tests/ and outside the default test
*
paths, with three benchmarks of the supervisor comms written as
plain pytest functions using pytest-codspeed (MIT);
*
a "benchmarks" dependency group;
*
a workflow that runs the benchmarks through CodSpeed in simulation
*
That mode counts CPU instructions rather than measuring time,
so results don't depend on how busy the shared runner is, and
CodSpeed comments each PR with its change against main.
CodSpeedHQ/action is already on the ASF allow-list (v5.2.1, pinned in
apache/infrastructure-actions). It authenticates through OIDC, so no
secret is needed. Someone with admin rights would still have to
enable the CodSpeed app for apache/airflow.
In the meantime
Following a suggestion in the contributors Slack, draft PR #74579 [9]
adds a "Performance-sensitive components" section to the
airflow-publish-changes agent skill. It lists the components that run
for every task, SDK call, log line or scheduler loop, and asks agents
to measure the changed path against the base commit before pushing
and to put the numbers in the PR description. It needs no tooling,
and can point at the benchmarks/ folders once they exist.
Questions I like your opinion on:
1.
Do we want micro-benchmarks for hot paths at all? If so, is this
*
an extension of AIP-59, or does it need its own AIP?
2.
Where should they live and how should they be named? I'd propose
*
<distribution>/benchmarks/, with paths mirroring the source module
*
they cover:
src/airflow/sdk/execution_time/comms.py
-> benchmarks/execution_time/test_comms_benchmark.py
CI could then run only the benchmarks covering the files a PR
changes.
1.
Which tool? pytest-codspeed with the hosted comparison, or
*
something purely local like pytest-benchmark? The local option
*
compares against a saved baseline with a threshold, but wall-time
*
thresholds on shared runners are noisy.
2.
What should a regression do: report only (a PR comment), or fail
*
the check above a threshold?
3.
Which paths should come first? Beyond the comms, candidates are
*
XCom serialisation, template rendering, DAG parsing, and the
*
scheduler's critical-section queries.
[1] https://github.com/apache/airflow/pull/62922
[2] https://github.com/apache/airflow/issues/71721
[3]
https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-59+Performance+tests+framework
[4] https://lists.apache.org/thread/0kyp33cp7q5m98s8q7c3pb6nobhprdyt
[5] https://github.com/apache/airflow/pull/41961
[6] https://github.com/apache/airflow/pull/41315
[7] https://lists.apache.org/thread/kg54mjdj1wg8r6jof28jrbjh7d3p7122
[8] https://github.com/apache/airflow/pull/74562
[9] https://github.com/apache/airflow/pull/74579
Thanks,
David
General (Internal Property)
