Revert "perf(pools): size image/rayon pools to the CFS quota"
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JG5yYZ9s868mJwqT2Qz7ez
This commit is contained in:
@@ -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
|
||||
|
||||
+3
-3
@@ -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::<usize>().ok())
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user