From 18781022bef1835d659a4b1d1dde9db0ba50c069 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 22 Jun 2026 09:04:37 +0000 Subject: [PATCH] Revert "perf(pools): size image/rayon pools to the CFS quota" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts the image-pool migration (commit 5629ba6). The bench (bench_pool_concurrency / POOL-CONCURRENCY.md) measured the one pool it could isolate — the thumbnail decode semaphore — and found flat throughput, p99 AND peak RSS (137 MiB) from K=1..16: shrink-on-load already makes each decode RAM-cheap, so sizing it to the CFS quota gains nothing measurable. Adding code without a measured benefit isn't worth it. Kept: the effective_parallelism() helper (it has a *measured* win in the Tokio runtime — benches/RUNTIME.md) and the benchmark itself (reusable). The ffmpeg video fan-out has a plausible a-priori case (one OS process per permit) but is left as a future, deliberately-measured change rather than shipped on speculation. Doc updated to record the decision. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01JG5yYZ9s868mJwqT2Qz7ez --- benches/POOL-CONCURRENCY.md | 22 +++++++++++-------- src/common/di.rs | 6 ++--- .../services/image_transcode_service.rs | 13 +++++------ .../services/thumbnail_service.rs | 12 +++++----- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/benches/POOL-CONCURRENCY.md b/benches/POOL-CONCURRENCY.md index 54b35499..b6f6dccc 100644 --- a/benches/POOL-CONCURRENCY.md +++ b/benches/POOL-CONCURRENCY.md @@ -56,15 +56,19 @@ taskset -c 0,1 ./target/release/examples/bench_pool_concurrency # model a 2-co every core". The flat RSS is exactly that: each concurrent decode's transient buffer is small, so 16 in flight cost the same resident memory as 1. -3. **So the pool migration is a correctness/consistency change, not a perf win.** - It is still worth keeping: it has **no downside** (off-quota `effective == - available`, so no change), it unifies pool sizing with the runtime fix behind - one `effective_parallelism()` helper, and it protects the pools this bench did - *not* isolate — the transcode rayon pool (thread stacks) and the ffmpeg video - fan-out (one OS process per permit), where over-spawning per *host* core under - a tight quota is genuinely wasteful. But operators should not expect a - throughput jump from it; the real download/runtime wins are in `BLOB-PREFETCH` - and `RUNTIME`. +3. **Decision: NOT migrated (reverted).** Because the only pool this bench could + isolate showed zero measured benefit, the `effective_parallelism()` migration + of the image pools was reverted — adding code without a measured win isn't + worth it. The `effective_parallelism()` helper stays (it has a *measured* + benefit in the Tokio runtime — see `RUNTIME`), so a future, deliberately + measured case can adopt it per-pool. + The one pool with a plausible a-priori argument is the **ffmpeg video + fan-out** (one heavyweight OS process per permit — 32 ffmpeg processes for a + 2-core budget on a many-core host is self-evidently wasteful). That was left + on `available_parallelism()` too, to revisit *with* a measurement if a + high-host-core / low-quota deployment running video thumbnails ever warrants + it. The transcode rayon pool over-sizing only costs parked thread stacks + (negligible). 4. **Honest caveat on scale.** This was run at a 2-core quota on a 4-core host (K_oversub = 8 ≈ 4×). On a 64-core host under a 2-core quota the host-count diff --git a/src/common/di.rs b/src/common/di.rs index 2e4c8db8..97f1c790 100644 --- a/src/common/di.rs +++ b/src/common/di.rs @@ -363,9 +363,9 @@ impl AppServiceFactory { if self.config.features.enable_video_thumbnails && FfmpegVideoFrameService::is_available(&ffmpeg_path) { - // effective_parallelism respects the CFS quota (--cpus), not - // just affinity — so ffmpeg fan-out matches the real core budget. - let cpus = crate::common::runtime::effective_parallelism(); + let cpus = std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(4); let concurrency = std::env::var("OXICLOUD_VIDEO_THUMBNAIL_CONCURRENCY") .ok() .and_then(|v| v.parse::().ok()) diff --git a/src/infrastructure/services/image_transcode_service.rs b/src/infrastructure/services/image_transcode_service.rs index 6b0b8bbe..4501f746 100644 --- a/src/infrastructure/services/image_transcode_service.rs +++ b/src/infrastructure/services/image_transcode_service.rs @@ -31,14 +31,13 @@ pub const MAX_TRANSCODE_SIZE: u64 = 5 * 1024 * 1024; /// Minimum number of threads in the dedicated transcoding pool const MIN_TRANSCODE_THREADS: usize = 2; -/// Compute the number of transcoding threads: half the available CPUs, with a -/// floor of `MIN_TRANSCODE_THREADS`. Sized by -/// [`effective_parallelism`](crate::common::runtime::effective_parallelism), -/// which respects CPU affinity **and** the CFS quota (Docker/K8s `--cpus`) — -/// unlike bare `available_parallelism()`, which ignores the quota and would -/// over-size this CPU-bound pool under a container limit. +/// Compute the number of transcoding threads: half the available CPUs, +/// with a floor of `MIN_TRANSCODE_THREADS`. `available_parallelism()` +/// respects cgroup limits (Docker/K8s) and CPU affinity masks. fn transcode_thread_count() -> usize { - let cpus = crate::common::runtime::effective_parallelism(); + let cpus = std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(MIN_TRANSCODE_THREADS); (cpus / 2).max(MIN_TRANSCODE_THREADS) } diff --git a/src/infrastructure/services/thumbnail_service.rs b/src/infrastructure/services/thumbnail_service.rs index 8d8e37cf..cbcd2b2a 100644 --- a/src/infrastructure/services/thumbnail_service.rs +++ b/src/infrastructure/services/thumbnail_service.rs @@ -107,11 +107,8 @@ const STREAM_TO_TEMP_TIMEOUT: Duration = Duration::from_secs(120); /// concurrency was halved to keep peak RAM in check. Decodes are now DCT-shrunk /// to the thumbnail size (~18–25 MB regardless of source resolution), so the RAM /// ceiling no longer forces throttling and we can saturate every core. Override -/// with `OXICLOUD_THUMBNAIL_DECODE_CONCURRENCY`. Sized by -/// [`effective_parallelism`](crate::common::runtime::effective_parallelism), -/// which respects CPU affinity **and** the CFS quota (`--cpus`) — so under a -/// container quota we don't over-permit concurrent CPU-heavy decodes onto cores -/// the scheduler can't give us. +/// with `OXICLOUD_THUMBNAIL_DECODE_CONCURRENCY`. `available_parallelism()` +/// respects cgroup limits (Docker/K8s) and CPU affinity masks. fn max_concurrent_decodes() -> usize { if let Some(n) = std::env::var(DECODE_CONCURRENCY_ENV) .ok() @@ -120,7 +117,10 @@ fn max_concurrent_decodes() -> usize { { return n; } - crate::common::runtime::effective_parallelism().max(2) + let cpus = std::thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(4); + cpus.max(2) } /// Thumbnail service for generating and caching image thumbnails