iframe-proxy

azat · 2026-05-05T15:34:51Z

Changelog category (leave one):

Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Per-CPU memory budget for max_untracked_memory to avoid OOMs

TBD

clickhouse-gh · 2026-05-05T15:35:31Z

Workflow [PR], commit [86f2f15]

Summary: ❌

job_name	test_name	status	info
Fast test		FAIL
	02818_memory_profiler_sample_min_max_allocation_size	FAIL	cidb
	04117_max_threads_min_free_memory_per_thread	FAIL	cidb
Build (amd_debug)		DROPPED
Build (amd_asan_ubsan)		DROPPED
Build (amd_tsan)		DROPPED
Build (amd_msan)		DROPPED
Build (amd_binary)		DROPPED
Build (arm_debug)		DROPPED
Build (arm_asan_ubsan)		DROPPED
Build (arm_tsan)		DROPPED
Build (arm_msan)		DROPPED

AI Review

Summary

This PR adds a per-CPU pacer for ThreadStatus::untracked_memory flushes using rseq or sched_getcpu, plus tests and a benchmark. I would request changes: the current code can still drop per-CPU budget charges when the rseq CPU id is invalid, header-defined storage still fragments state across translation units, Linux fallback configurations are still rejected by a test, and the Performance Improvement claim still lacks reproducible before/after evidence despite a reported Cloud benchmark regression.

PR Metadata

💡 Changelog category as Performance Improvement matches the intent only if the performance claim is backed by measurements. The Changelog entry is required and should not leave the trailing TBD. Suggested entry: Added a per-CPU flush budget for max_untracked_memory to reduce aggregate untracked memory in highly concurrent queries.

Findings

⚠️ Majors

[src/Common/PerCPUMemoryBudget.h:151] chargeAlloc / chargeFree zero state.pending_alloc / state.pending_free before detail::charge; when rseq_ready is true but rseq_cpu_start returns -1 / -2, this branch returns false and drops that chunk from the per-CPU budget instead of charging it through the sched_getcpu fallback. That weakens the PR's bound exactly in a documented rseq unavailable state. Fall back to the atomic path or restore the pending bytes before returning.
[src/Common/PerCPUMemoryBudget.h:82] slots is still a namespace-scope const definition in a header without inline / extern, so each translation unit that includes PerCPUMemoryBudget.h gets a separate slot array. Today the direct unit tests and CurrentMemoryTracker.cpp do not share one per-CPU state, and any additional caller would fragment the intended process-wide budget. Move storage to one .cpp with extern declarations, or make the variable explicitly inline if header storage is intended. I attempted to reopen the existing bot-authored thread, but GitHub returned Resource not accessible by integration.
[dismissed by author -- https://github.com/Per-CPU memory budget for max_untracked_memory to avoid OOMs #104125#discussion_r3190013841] [src/Common/tests/gtest_per_cpu_memory_budget.cpp:41] The test still hard-requires PerCPUMemoryBudget::isRSeqReady on every Linux run, while the implementation has an explicit sched_getcpu fallback when rseq is unavailable. This can fail valid Linux configurations where per-CPU accounting still works through the fallback.
[dismissed by author -- https://github.com/Per-CPU memory budget for max_untracked_memory to avoid OOMs #104125#discussion_r3299482355] [src/Common/CurrentMemoryTracker.cpp:85] The PR is categorized as Performance Improvement, but there are still no reproducible before/after numbers for the hot CurrentMemoryTracker::allocImpl / CurrentMemoryTracker::free paths, and the PR discussion includes Cloud benchmark regression reports. The performance contract remains unproven.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions: preserve or fallback-charge pending bytes on invalid rseq CPU ids, make the per-CPU slot storage shared or explicitly confine/test the single-TU contract, fix or gate the Linux rseq expectation in the test, and attach reproducible benchmark evidence addressing the reported regression.

yurifedoseev · 2026-05-06T09:25:33Z

Cloud Benchmarks reports showed some degradation in the benches https://c.house/eo5XwkUdhU

Introduces per-CPU memory budget - a per-CPU slice that mirrors the existing per-thread `untracked_memory` accumulator on a per-CPU so `Σ untracked_memory` is bounded by O(ncpu) instead of O(nthreads). `total_memory_tracker` therefore becomes a true upper bound on real memory usage, which should prevent OOMs (real one from kernel) due to untracked_memory. rseq is used for better performance Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

yurifedoseev · 2026-05-17T13:42:10Z

@azat Cloud Benchmarks report shows a big regression https://c.house/eo5XwkUdhU

clickhouse-gh · 2026-05-27T14:46:28Z

+    {
+        cpu = static_cast<int>(rseq_cpu_start());
+        if (unlikely(static_cast<unsigned>(cpu) >= static_cast<unsigned>(slot_count)))
+            return false;


⚠️ When rseq_ready is true but rseq_cpu_start returns an invalid CPU id (-1/-2), this branch returns false immediately.

At that point chargeAlloc / chargeFree have already zeroed state.pending_alloc / state.pending_free, so this chunk is dropped from per-CPU accounting instead of falling back to the sched_getcpu + atomic path.

That weakens the budget exactly in the per-thread-unavailable-rseq case. Can we fall back to the non-rseq path here instead of returning early?

clickhouse-gh · 2026-05-27T18:12:50Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.10%	84.10%	+0.00%
Functions	91.40%	91.40%	+0.00%
Branches	76.60%	76.50%	-0.10%

Changed lines: 96.10% (197/205) · Uncovered code

Full report · Diff report

azat · 2026-05-28T14:37:14Z

clickhouse-gh Bot added pr-performance Pull request with some performance improvements submodule changed At least one submodule changed in this PR. labels May 5, 2026

azat changed the title ~~Per-CPU memory budget for max_untracked_memory~~ Per-CPU memory budget for max_untracked_memory to avoid OOMs May 5, 2026