Perf: threaded minion (multiprocessing: False) rebuilds all execution modules on every job

[ggiesen]

Summary

On a threaded minion (multiprocessing: False), Minion._thread_return calls minion_instance.gen_modules() at the top of every job, which rebuilds and rebinds the entire execution-module loader. On a stock install that is ~1960 execution functions rebuilt per job, measured at ~21 ms/job of pure loader-construction overhead (see below).

PR #69755 fixes the correctness bug this rebuild caused (#61830 / #63117 -- concurrent jobs stealing each other's retcode) by having each job own the loader it wrote its retcode into. It deliberately keeps the per-job rebuild, because removing it is a separate, higher-risk change. This issue tracks that follow-up.

Why the per-job rebuild exists

The per-job gen_modules() call was added in 9f1fe42b3c ("Call os.fork less to avoid race conditions") as a fork-race mitigation. On a forking minion (multiprocessing: True) each job is its own process, so the rebuild is moot. On a threaded minion the rebuild's one load-bearing effect is giving each job a private __context__ dict, so concurrent jobs don't corrupt each other's __context__["retcode"] -- which is exactly the #61830 mechanism.

If __context__ were isolated per-job by a cheaper mechanism, the full loader rebuild could be dropped entirely on the threaded path.

Performance measurement

If you run a threaded minion with light, high-frequency jobs and want to cut per-job latency, dropping the per-job rebuild is the lever. Measured before/after, single-tenant:

A. Cost of the rebuild in isolation -- a single real gen_modules() call, the exact work _thread_return does per job:

REAL gen_modules() per job: median=21.2ms  mean=21.4  min=18.4  max=53.1  (n=25)

Stable across 3 independent process runs (medians 21.2 / 21.4 / 21.2 ms). Real salt.loader.minion_mods(opts) build with real grains, warm import caches, no competing host load, median of 25 iterations. 1958 execution functions on this host; construction cost scales ~linearly with module count, so sites with more _modules/custom modules pay more.

B. Per-job latency, with the rebuild vs without -- same process, same host, the only variable is whether the job rebuilds the loader (status quo) or reuses a persistent one (the proposed change), timing resolve-and-call of a trivial job (test.ping):

with per-job rebuild (today):    median=21.1ms/job
without rebuild (proposed):      median=0.005ms/job
per-job latency reduction:       ~21ms/job

Stable across 3 runs (21.1 / 20.7 / 21.7 ms with rebuild). For a light job the ~21 ms rebuild is essentially all of the per-job processing overhead, so removing it takes that overhead to near zero.

How to read it: the ~21 ms is a fixed per-job tax, not proportional to the job, so the relative win is inversely proportional to job weight:

• high-frequency light jobs (test.ping, grains.get, scheduled beacons) are dominated by it -- ~21 ms/job reduction is the bulk of their wall time;
• a ~200 ms job saves ~10%;
• a multi-second job saves a negligible fraction.

Note: PR #69755 does not change this cost -- the rebuild has been on the threaded path since 2019. #69755 is correctness-neutral on performance; this issue is what removes the cost.

Proposed direction

Isolate __context__ per job with a module-level contextvars.ContextVar instead of a fresh loader per job: _thread_return / _thread_multi_return set a fresh context at job entry and reset it on exit, and the per-job gen_modules() rebuild is removed. A throwaway prototype of this shape ran clean (0 retcode corruption up to 16 concurrent workers x 100 jobs = 1600 jobs), confirming the approach closes the same race #69755 does while dropping the rebuild.

This is a re-architecture of __context__ isolation, not a small patch, and it must handle several things the prototype skipped:

• sys.reload_modules -- currently bound to gen_modules; Fix wrong retcodes from concurrent jobs on a threaded minion #69755 already decouples it to a wrapper that returns None, which this can build on.
• the state and render loaders, which also pack __context__.
• the known latent thread-safety defects in the existing ChildContextDict / StackContext machinery (the older, deprecated context-isolation path).
• it should be evaluated with Python free-threading (PEP 703) in mind, since that changes the threaded-minion story materially.

Why this is separate from #69755

Correctness (wrong retcodes silently reported as success) is the urgent, low-risk fix and ships now. The performance re-architecture is independent, touches shared context machinery, and carries its own review risk. Keeping them separate isolates that risk and lets the correctness fix land without waiting on the larger change.