mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add Windows unified exec yield floor (#27086)
## Why The Windows `unified_exec` experiment regressed at the turn level in a way that points to premature backgrounding / extra command cycles rather than individual responses getting heavier: - `codex_local_tool_calls_per_turn` was up about 20.7%. - `codex_local_blended_tokens_per_turn` was up about 4.1%, and `codex_local_output_tokens_per_turn` was up about 4.0%. - `codex_local_response_latency_per_turn` was up about 8.3%. - The primary activity metrics also moved down: `codex_turns` about -6.6%, `codex_dau` about -1.0%, and `codex_local_hourly_active_users` about -3.0%. At the same time, the per-response metrics moved in the other direction: blended tokens per response, output tokens per response, and latency per response were all lower in test. That suggests the bad turn-level shape is largely about extra tool/model cycles, not each response being slower or more expensive on its own. Local Windows benchmarking showed the likely mechanism: shell-wrapped commands pay a large PowerShell startup/teardown tax before the actual command has much time to run. In the benchmark, the PowerShell wrapper added roughly 0.7-1.0s versus direct exec: - Windows PowerShell: about 740ms p50 / 800ms p90 overhead versus direct exec. - PowerShell 7 (`pwsh`): about 930ms p50 / 980ms p90 overhead versus direct exec. The model commonly asks for a 1s initial yield. On Windows, that can spend nearly the whole window waiting on PowerShell machinery, so otherwise-short commands are more likely to return as background sessions and require follow-up polling/tool calls. This is intentionally a temporary unlock. It gives Windows closer to the same useful post-shell command window as other platforms while we work on reducing the PowerShell tax directly, for example with persistent PowerShell workers or conservative direct-exec paths for commands that do not need shell semantics. ## What changed - Adds a Windows-only 2s floor to `unified_exec`'s initial `yield_time_ms` clamp. - Keeps larger model-requested waits unchanged, including the existing 10s default. - Keeps the existing 30s max clamp. - Leaves non-Windows behavior unchanged. - Adds platform-gated tests for both the Windows floor and the non-Windows clamp behavior. ## Verification - `just test -p codex-core unified_exec`
This commit is contained in:
committed by
GitHub
Unverified
parent
c0b36d234a
commit
e7a9988d1a
@@ -62,6 +62,7 @@ pub(crate) use process::SpawnLifecycleHandle;
|
||||
pub(crate) use process::UnifiedExecProcess;
|
||||
|
||||
pub(crate) const MIN_YIELD_TIME_MS: u64 = 250;
|
||||
pub(crate) const WINDOWS_INITIAL_EXEC_YIELD_TIME_FLOOR_MS: u64 = 2_000;
|
||||
// Minimum yield time for an empty `write_stdin`.
|
||||
pub(crate) const MIN_EMPTY_YIELD_TIME_MS: u64 = 5_000;
|
||||
pub(crate) const MAX_YIELD_TIME_MS: u64 = 30_000;
|
||||
@@ -165,6 +166,11 @@ struct ProcessEntry {
|
||||
}
|
||||
|
||||
pub(crate) fn clamp_yield_time(yield_time_ms: u64) -> u64 {
|
||||
let yield_time_ms = if cfg!(windows) {
|
||||
yield_time_ms.max(WINDOWS_INITIAL_EXEC_YIELD_TIME_FLOOR_MS)
|
||||
} else {
|
||||
yield_time_ms
|
||||
};
|
||||
yield_time_ms.clamp(MIN_YIELD_TIME_MS, MAX_YIELD_TIME_MS)
|
||||
}
|
||||
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use super::*;
|
||||
use crate::unified_exec::clamp_yield_time;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tokio::time::Duration;
|
||||
use tokio::time::Instant;
|
||||
@@ -131,6 +132,32 @@ fn exec_server_process_id_matches_unified_exec_process_id() {
|
||||
assert_eq!(exec_server_process_id(/*process_id*/ 4321), "4321");
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn initial_exec_yield_time_uses_windows_floor() {
|
||||
let above_max_yield_time_ms = crate::unified_exec::MAX_YIELD_TIME_MS + 1;
|
||||
|
||||
assert_eq!(
|
||||
clamp_yield_time(/*yield_time_ms*/ 1_000),
|
||||
crate::unified_exec::WINDOWS_INITIAL_EXEC_YIELD_TIME_FLOOR_MS
|
||||
);
|
||||
assert_eq!(clamp_yield_time(/*yield_time_ms*/ 10_000), 10_000);
|
||||
assert_eq!(
|
||||
clamp_yield_time(/*yield_time_ms*/ above_max_yield_time_ms),
|
||||
crate::unified_exec::MAX_YIELD_TIME_MS
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
#[test]
|
||||
fn initial_exec_yield_time_has_no_platform_floor() {
|
||||
assert_eq!(clamp_yield_time(/*yield_time_ms*/ 1_000), 1_000);
|
||||
assert_eq!(
|
||||
clamp_yield_time(/*yield_time_ms*/ 1),
|
||||
crate::unified_exec::MIN_YIELD_TIME_MS
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn network_denial_fallback_message_names_sandbox_network_proxy() {
|
||||
let message = network_denial_message_for_session(/*session*/ None, /*deferred*/ None).await;
|
||||
|
||||
Reference in New Issue
Block a user