Repository navigation
perf(ui): chart sections keep their per-publication series and window maxima between samples, so a frame between samples rebuilds only x (#843) - #2937
Merged
Conversation
… maxima between samples (#843) Between samples every history chart redraws the same publication each frame; only x moves with now. What still ran every frame for data that had not changed: - The window maxima behind every rate/percent axis (maxOfSeriesSince(), a scan of every visible sample of every series): now windowSeriesMax(), a VisibleMaxCache per axis keyed on the data generation, the series, the axis length and the window's first sample, so a frame between samples pays a binary search. Network, Disk I/O (aggregate and per disk), Memory peak, GPU clock reference, Overview power/resources and every Process Details axis. - The stacked CPU charts' band edges and lines (Overview, Process Details): gathered once per choice of points (ReducedValuesStamp), with the held-to-now value appended; each frame rebuilds only x. - The GPU clock/temperature/power lines scaled to percent: a PercentSeriesCache per GPU, rebuilt for a new generation or scale. - The time axis's tick labels: formatDuration() built them with std::format, which in MSVC's library reserves past the short-string buffer, so every tick of every chart allocated every frame. It now formats into a local buffer. Values, reduced points, hover and tooltips are unchanged: each cache returns exactly what the per-frame computation did, and tests pin that. Part of #843. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
13 of 31 tasks
…charts' values came from, and their draw count never passes a series' end (#843) Review follow-up on the stacked CPU charts' kept values. ReducedValuesStamp compared the ReducedPointsCache's rebuild count, which starts over at 0 when the cache is reset or replaced (or a frame-keyed entry is pruned and made again). After as many rebuilds the count matched, so the old values passed for the new points' while x was rebuilt from them, and drawing x.size() points could read past the end of a band edge. Each rebuild now takes a process-unique pointsId(), which the stamp compares. As a second guard, the Overview, Process Details and harness stacks draw drawablePointCount() points: x's length, or a shorter series' length should one ever be shorter. Tests: a cache reset between frames regathers (unit and in the stacked scene); drawablePointCount() never passes the shortest series. Also a render test pinning every Process Details axis (Overview, Network and I/O, GPU, NPU) to the bound its helper's series form gives, run once per history in which each otherwise dominated series (User, System, Shared, Private, Swap, GDI Objects, I/O Read, Net Sent, NPU Dedicated) is the largest, so a series dropped from an axis fails it. Part of #843. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mgradwohl
enabled auto-merge (squash)
October 25, 2026 15:37
This was referenced Oct 25, 2026
mgradwohl
added a commit
that referenced
this pull request
Oct 27, 2026
…Widgets.h (#3116) (#3134) reduceSeriesKeepingGaps, reduceSeriesMinMax, reduceAlignedSeries and holdLastValuesToNow, and writeReducedPoint behind the first two, had no caller under src/, benchmarks/ or the fuzzers: only tests used them. They move unchanged to tests/UI/ChartReferenceHelpers.h, in UI::Widgets::TestReference, so test_ChartWidgets.cpp keeps its peak, gap, alignment and hold-to-now tests and #2937's check of appendHeldValues + holdTimeToNow against holdLastValuesToNow. The two test_ProcessDetailsHistory.cpp tests that held a series to now now use the charts' own appendHeldValues + holdTimeToNow, with the same assertions. The descriptions the removed wrappers carried move to the production primitives (forEachStrideReducedPoint, forEachAlignedReducedPoint, holdTimeToNow, appendHeldValues), and the comments that pointed at the removed names point at those. No include of ChartWidgets.h was used only by the moved helpers. No production behaviour changes. Fixes #3116 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is the "Per-frame chart data/vector churn" item of #843, Phase 3.
What was already done. The plan's text is from 2025-09, so I checked the current code first.
buildTimeAxisno longer exists: every chart takes its axis fromframeTimeAxis(), whoseTimeAxisPoolreuses its buffers between frames (#1018). The sections read the published history through spans, with no per-frame copies (#1018). Their scratch vectors and labels already live in caches kept across frames (#1171), and reduced points are cached per generation (ReducedPointsCache, #1139). So, measured in Release, no chart-data path allocated per frame before this PR. What still ran every frame was work on data that hadn't changed, plus one allocating formatter.What this changes. Between samples, each chart now redraws a publication's data from caches; only x is recomputed, since it moves with now:
maxOfSeriesSince()scanned every visible sample of every serieswindowSeriesMax(): aVisibleMaxCacheper axis (ImGui-ID scoped, so per-disk/per-GPU axes keep their own), keyed on the data generation, the series (address + length), the axis length and the index of the window's first sample. A frame between samples does one binary search. It rescans only for a new generation or when a sample crosses the left edgeReducedValuesStamp, keyed on theReducedPointsCacherebuild), with the held-to-now value appended (appendHeldValues()). Each frame rebuilds x (gatherReducedTimes()) and appends x = 0 only when the last sample holds (holdTimeToNow()). Drawingx.size()points then draws exactly whatholdLastValuesToNow()didSystemMetricsPanel), Process Details CPU (ProcessDetailsCharts), and the geometry harness's scene that mirrors the OverviewnormalizeToPercent(), O(history) × 3 per GPU) into scratch buffers shared by all GPUsPercentSeriesCache, one per GPU per line, rebuilt for a new generation, history or scaleGpuSectionformatDuration()usedstd::format, which in MSVC's library reserves past the short-string buffer even for "5m"Format::Detail::formatShort()formats into a 32-byte local buffer, so the string fits SSOUI/Format.h(all callers offormatDuration())Already fine, left unchanged: CpuCoresSection's per-core data is drawn through
plotLineWithFill(), which caches its own reduction. Its per-frame work is the shared pooled axis and the static grid/name/kind caches. StorageSection's per-disk frames already reuse static buffers (#1171).No behaviour change. Every cache returns exactly what the per-frame computation returned: the same values, reduced points, hover and tooltips. The cached window max is a pure function of its key. The stack's y values depend only on the points and data that the
ReducedPointsCachekey names. The percent series depend on their key. The tests below pin that equivalence.Part of #843
PR Size
Large: 21 files, +1,039 / −193, of which about 500 lines are tests. The source diff is mostly the single-line call-site swaps (
maxOfSeriesSince→windowSeriesMax). It also gives each Process Details axis helper a "from the window's max" overload; the span form delegates to it, and the existing tests still use the span form.Type of Change
Checklist
clang-tidyand addressed any warningsclang-formaton my changespre-commit run --all-filesor installed pre-commit hooks (pre-commit run --files <touched>: all passed)ChartWidgets.hnamesvisibleMaxCache())Testing
New tests. Each cache gets a "a second frame with the same version doesn't rebuild, a new version does" test, and each one-to-one replacement gets an equivalence test:
test_RateAxis.cpp,VisibleMaxCacheTest:maxOfSeriesSince()every frame, with exactly five scans (the first, then one per left-edge crossing).test_ChartFrameCaches.cpp(new), in headless ImGui frames:windowSeriesMax()scans once per generation;PushIDscopes keep their own max;test_ChartWidgets.cpp:ReducedValuesStampis stale only after a points rebuild (new generation or budget), and every frame at generation 0;appendHeldValues()+holdTimeToNow()draw exactly whatholdLastValuesToNow()did, for held, too-old and at-now last samples, NaN gaps included;gatherReducedTimes()is covered;buildUserSystemStacktests run on the split API.test_GpuSeriesScale.cpp,GpuPercentSeriesTest: no rebuild for the same history and scale; rebuilds for a new generation, scale or history; generation 0 rebuilds every call; values equalnormalizeToPercent(), gaps included.test_GpuSectionRender.cpp: three frames of one GPU scale clock, temperature and power once each, and a new generation scales them once more.test_Format.cpp:formatShort()matchesstd::formatand cuts at its buffer; the longest duration (the 1e15 s cap) comes out whole.One launch (
TASKSMACK_SYNTHETIC, a scratchTASKSMACK_CONFIG_DIR,TASKSMACK_SCREENSHOT=...;after=65; it quit by itself, exit 0). In the Overview, the CPU bands and lines reach "now", and every axis and its bars agree.Measurements
#2926 (
TASKSMACK_RENDER_METRICS) is still open, so I measured with a headless harness instead of the overlay CSV. It's a scratch program, not committed. It renders the real sections through their real render functions:ChartGeometryScenes.h, which this PR keeps in step with the panel.It runs 60 publications × 60 frames (a 60 s run at 1 s refresh, 60 fps) after a warm-up publication. A global
operator newhook counts allocations, and each frame is timed whole (NewFrame → Render). Two history sizes: 60 samples (a 60 s window at 1 s), and 1,800 (a 30-minute window at 1 s). The build is win-release (clang 23, -O3, ThinLTO), base 012b6f2 against this branch. The runs are interleaved, 4 each, and the table gives medians.Debug numbers aren't used: there, every container construction allocates an MSVC debug-iterator proxy.
formatDuration()change): Process Details Overview 88.2 → 72.9 µs, Network 128.9 → 116.7 µs, every allocation count unchanged. So the CPU win comes from the caches, and the allocation win from the tick labels.clang-tidy
.\tools\clang-tidy.ps1 -Files ...on host LLVM 23. It found three issues in this change, now fixed:misc-include-cleaner:UI/RateAxis.hin MemorySection.cpp is no longer used directly;<cstdint>in test_GpuSeriesScale.cpp;<cstddef>in test_RateAxis.cpp.What's left is LLVM-23-only or in code this PR doesn't touch:
readability-trailing-comma,misc-const-correctnesson lambdas,readability-redundant-lambda-parameter-list,bugprone-signed-bitwiseon ImGui flag enums, and existing test-file findings.Additional Notes
Found but not changed:
UI::InlineText([PERF] Remaining per-frame heap allocations on chart and panel paths #1171) would remove them.m_ChartDataGenerationinSystemMetricsPanelis one generation for every source (system, storage, GPU, process histories). A storage or GPU publication therefore invalidates the CPU and memory charts' caches too, about 3–4 times a second at 1 s refresh instead of once. That is still far from every frame. A generation per source would cut it further.plotLineWithFill()still gathers y from its kept points every frame, at most the plot's point budget per series. Keeping the gathered y inReducedPointsCachewould remove that too, but it's the generic hot path for every series, so I left it for a measured change of its own.holdLastValuesToNow()has no production caller left. I kept it as the tested reference semantics forappendHeldValues()+holdTimeToNow().🤖 Generated with Claude Code