chore: viewport-aware downsampling and 60fps interaction for minimap#467
Merged
chore: viewport-aware downsampling and 60fps interaction for minimap#467
Conversation
Contributor
Review Summary by QodoViewport-aware downsampling and 60fps throttled rendering for minimap interaction
WalkthroughsDescription• Implement viewport-aware MinMax downsampling for main plot rendering ~4,000 points per channel regardless of dataset size • Add 60fps render throttling via DispatcherTimer to eliminate redundant re-renders during minimap drag and main plot pan/zoom • Introduce binary search (FindVisibleRange) and sub-range Downsample overload for O(log n) viewport extraction • Replace 1M point hard cap with 50M practical memory limit using SQL-level .Take() to avoid materializing excess data • Add composite database index on (LoggingSessionID, TimestampTicks) for faster ordered session queries • Fix ResetZoom to compute full data range from source data instead of relying on auto-range from downsampled ItemsSource • Eliminate feedback loop between minimap drag and main axis AxisChanged event using IsSyncingFromMinimap guard flag • Reuse cached List<DataPoint> per series during interaction to eliminate GC pressure (~960 allocations/sec) • Use LineSeries.Tag for key lookup instead of fragile title-string parsing • Add 11 unit tests covering downsampling, sub-range operations, binary search, and performance benchmarks Diagramflowchart LR
A["Main Plot Interaction"] -->|"pan/zoom"| B["OnMainTimeAxisChanged"]
B -->|"mark dirty"| C["ViewportThrottleTimer"]
C -->|"60fps tick"| D["UpdateMainPlotViewport"]
D -->|"FindVisibleRange"| E["Binary Search"]
E -->|"get indices"| F["Downsample Sub-range"]
F -->|"reuse cache"| G["Update ItemsSource"]
G -->|"InvalidatePlot"| H["OxyPlot Render"]
I["Minimap Drag"] -->|"ApplyToMainPlot"| J["IsSyncingFromMinimap=true"]
J -->|"Zoom axis"| K["RenderThrottleTimer"]
K -->|"60fps tick"| L["OnMinimapViewportChanged"]
L -->|"UpdateMainPlot"| D
M["DisplayLoggingSession"] -->|"SQL Take"| N["MAX_IN_MEMORY_POINTS"]
N -->|"populate"| O["_allSessionPoints"]
O -->|"Downsample"| P["_downsampledCache"]
P -->|"set"| G
File Changes1. Daqifi.Desktop.Test/Helpers/MinMaxDownsamplerTests.cs
|
Contributor
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+454
to
+469
| var baseQuery = context.Samples.AsNoTracking() | ||
| .Where(s => s.LoggingSessionID == session.ID); | ||
|
|
||
| var samplesCount = dbSamples.Count; | ||
| const int dataPointsToShow = 1000000; | ||
| var totalSamplesCount = baseQuery.Count(); | ||
|
|
||
| if (samplesCount > dataPointsToShow) | ||
| if (totalSamplesCount > MAX_IN_MEMORY_POINTS) | ||
| { | ||
| subtitle = $"\nOnly showing {dataPointsToShow:n0} out of {samplesCount:n0} data points"; | ||
| subtitle = $"\nShowing first {MAX_IN_MEMORY_POINTS:n0} of {totalSamplesCount:n0} data points"; | ||
| } | ||
|
|
||
| // Only materialize up to the limit to avoid excessive memory usage | ||
| var dbSamples = baseQuery | ||
| .OrderBy(s => s.TimestampTicks) | ||
| .Select(s => new { s.ChannelName, s.DeviceSerialNo, s.Type, s.Color, s.TimestampTicks, s.Value }) | ||
| .Take(MAX_IN_MEMORY_POINTS) | ||
| .ToList(); |
Contributor
There was a problem hiding this comment.
1. displayloggingsession uses sync ef 📘 Rule violation ➹ Performance
DisplayLoggingSession performs synchronous EF Core queries (Count() and ToList()), which can block the calling thread (potentially the UI thread) during session load. This violates the requirement to use async DB methods for responsiveness and scalability.
Agent Prompt
## Issue description
`DisplayLoggingSession` uses synchronous EF Core query execution (`Count()`, `ToList()`), which can block threads during large session loads.
## Issue Context
Compliance requires async database operations where appropriate to avoid blocking UI and improve responsiveness.
## Fix Focus Areas
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[454-469]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Fixed. Switched from ToList() to AsEnumerable() streaming — samples are now iterated directly from the EF query enumerator into per-channel lists without materializing a large intermediate list. This eliminates the peak memory spike from the anonymous object allocation.
Note: DisplayLoggingSession is called via BackgroundWorker.DoWork, so the sync EF calls don't actually block the UI thread. Converting to async would require changing the caller to Task.Run + await, which is a broader refactor. The streaming change addresses the immediate memory concern without that scope change.
Also reduced MAX_IN_MEMORY_POINTS from 50M to 10M (~160MB for DataPoints) as a more defensible default.
Contributor
Author
There was a problem hiding this comment.
Disagree. DisplayLoggingSession is called from a BackgroundWorker thread (see DaqifiViewModel.cs:1212), not the UI thread. The Dispatcher.Invoke calls within it confirm this — they marshal UI updates from the background thread. Converting to async would require changing the caller's threading model for no practical benefit since the calling thread is already a background thread. SQLite also doesn't benefit from async I/O (it's file-backed, not network I/O).
Comment on lines
+454
to
+469
| var baseQuery = context.Samples.AsNoTracking() | ||
| .Where(s => s.LoggingSessionID == session.ID); | ||
|
|
||
| var samplesCount = dbSamples.Count; | ||
| const int dataPointsToShow = 1000000; | ||
| var totalSamplesCount = baseQuery.Count(); | ||
|
|
||
| if (samplesCount > dataPointsToShow) | ||
| if (totalSamplesCount > MAX_IN_MEMORY_POINTS) | ||
| { | ||
| subtitle = $"\nOnly showing {dataPointsToShow:n0} out of {samplesCount:n0} data points"; | ||
| subtitle = $"\nShowing first {MAX_IN_MEMORY_POINTS:n0} of {totalSamplesCount:n0} data points"; | ||
| } | ||
|
|
||
| // Only materialize up to the limit to avoid excessive memory usage | ||
| var dbSamples = baseQuery | ||
| .OrderBy(s => s.TimestampTicks) | ||
| .Select(s => new { s.ChannelName, s.DeviceSerialNo, s.Type, s.Color, s.TimestampTicks, s.Value }) | ||
| .Take(MAX_IN_MEMORY_POINTS) | ||
| .ToList(); |
Contributor
There was a problem hiding this comment.
3. 50m load can oom 🐞 Bug ☼ Reliability
DisplayLoggingSession can materialize up to 50,000,000 rows into an in-memory list via ToList() and then expand that into DataPoint lists, which can exhaust memory and crash the app on long/high-rate sessions. The MAX_IN_MEMORY_POINTS limit is too high given the intermediate allocations and object overhead.
Agent Prompt
### Issue description
`DisplayLoggingSession` does `Take(MAX_IN_MEMORY_POINTS).ToList()` with `MAX_IN_MEMORY_POINTS = 50_000_000`, then loops that list to build `_allSessionPoints`. This can allocate an extremely large intermediate list plus large per-channel point lists and can easily OOM.
### Issue Context
The PR intent is to avoid unbounded materialization; however, 50M eagerly materialized rows is still effectively unbounded for typical desktops, especially with anonymous-object overhead.
### Fix Focus Areas
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[103-109]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[454-495]
### Suggested fix approach
- Reduce `MAX_IN_MEMORY_POINTS` to a defensible default (or make it configurable).
- Avoid `ToList()` for the full sample set:
- Run a small query for channel metadata (distinct channels) first.
- Stream samples (e.g., `AsAsyncEnumerable()` / pagination by timestamp) and append directly into per-key lists, or build per-channel lists in batches.
- If you must cap, cap per-channel and/or cap total points after downsampling rather than after raw materialization.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Fixed. Two changes:
-
Reduced cap from 50M to 10M — 10M DataPoints ≈ 160MB, which is reasonable for desktop use. This covers 16 channels × 1000Hz × ~10 minutes of data, which is a practical session size.
-
Streaming instead of ToList() — replaced
.Take(MAX_IN_MEMORY_POINTS).ToList()with.Take(MAX_IN_MEMORY_POINTS).AsEnumerable()and a foreach loop that appends directly into per-channel lists. This eliminates the intermediate anonymous object list (which was the main OOM risk — the anonymous objects have higher per-item overhead than DataPoint structs).
The channel metadata query is now a separate small query (Distinct() on channel info fields) that materializes only the channel list, not the sample data.
Contributor
Author
There was a problem hiding this comment.
Already fixed in commits 8c5b8db and 700e52d. The 50M MAX_IN_MEMORY_POINTS constant was removed entirely. We now use two-phase progressive loading: Phase 1 loads 100K samples via index scan, Phase 2 loads ~3000 sampled points per channel via targeted index seeks. Total in-memory footprint is ~96K points regardless of session size — far below OOM risk.
tylerkron
changed the title
Contributor
Author
|
Addressing items 4-7 from the code review summary: 4. Sync flag not exception-safe — Fixed. Wrapped 5. Perf test may be flaky — Fixed. Relaxed the threshold from 100ms to 1000ms. The test is a sanity check that binary search is O(log n) rather than O(n), not a precise benchmark. 1000ms is generous enough for slow CI while still catching algorithmic regressions (O(n) on 1M points × 1000 iterations would take seconds). 6. Subrange Downsample missing bounds validation — Fixed. Added 7. Index not applied to existing DBs — Acknowledged, no code change. This is correct — |
…imap - Add viewport-aware MinMax downsampling for the main plot so OxyPlot never renders more than ~4000 points per channel regardless of dataset size - Throttle minimap interaction renders at 60fps via DispatcherTimer instead of invalidating both plots on every mouse move - Break the feedback loop between minimap drag and main axis AxisChanged event using a guard flag (IsSyncingFromMinimap) - Add binary search (FindVisibleRange) and sub-range Downsample overload to MinMaxDownsampler for O(log n) viewport extraction - Replace 1M hard point cap with 50M limit using SQL-level Take() to avoid materializing entire result sets into memory - Add composite DB index on (LoggingSessionID, TimestampTicks) for faster session queries - Use LineSeries.Tag for key lookup instead of fragile title-string parsing - Remove unused _sessionPoints dictionary - Add 11 unit tests for MinMaxDownsampler (binary search, sub-range, perf) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After viewport downsampling, ItemsSource contains only the visible subset. ResetAllAxes() auto-ranges from this subset, causing the plot to appear clipped to the last zoomed range. Fix by restoring full-range downsampled data before resetting axes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MinMax downsampling doesn't preserve exact first/last X values (it emits points at min/max Y positions within each bucket). When OxyPlot auto- ranges from downsampled data, the axis range is narrower than the actual data, causing a cascading clip effect through OnMainTimeAxisChanged. Fix by computing the full data range from _allSessionPoints and explicitly setting the time axis via Zoom(), while only auto-ranging the Y axes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pdate InvalidatePlot(false) only re-renders from OxyPlot's internal cache, ignoring changes to ItemsSource. After zoom button + minimap drag, the main plot showed missing data because the updated downsampled data was never picked up. Changed to InvalidatePlot(true) in both OnRenderTick and OnMouseUp to ensure OxyPlot reads the fresh ItemsSource. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two performance improvements for sustained 60fps during interaction: 1. Reuse cached List<DataPoint> per series in UpdateMainPlotViewport() instead of allocating new lists every frame. With 16 channels at 60fps, this eliminates ~960 list allocations/sec that were causing GC micro-stutters. 2. Throttle viewport updates from main plot pan/zoom to 60fps via DispatcherTimer + dirty flag (matching the minimap's approach). Previously, OnMainTimeAxisChanged called UpdateMainPlotViewport() synchronously on every mouse move event (~120Hz). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Records hard-won lessons from the minimap performance work: - Why viewport-aware downsampling over global LTTB decimation - InvalidatePlot(true) vs (false) and when each is required - MinMax downsampling X-boundary drift causing auto-range shrinkage - Minimap ↔ main plot feedback loop prevention pattern - GC pressure from per-frame list allocations - DispatcherTimer + dirty flag throttle pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ADR 001 documents why we chose viewport-aware MinMax downsampling over global LTTB (PR #457), pre-computed pyramids, GPU rendering, and on-demand DB queries. Records the trade-offs, consequences, and follow-up work for future contributors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reduces context window usage by replacing detailed explanations with bullet-point gotchas and linking to ADR 001 for the full rationale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. Stream samples via AsEnumerable() instead of ToList() to avoid materializing a large intermediate anonymous object list (OOM risk) 2. Reduce MAX_IN_MEMORY_POINTS from 50M to 10M (~160MB) as a more defensible default for desktop memory constraints 3. Separate channel metadata query from sample streaming 4. Add IDisposable to DatabaseLogger — stops viewport throttle timer, disposes minimap interaction controller, buffer, and consumer gate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4. Wrap IsSyncingFromMinimap flag set/unset in try/finally to ensure the flag is reset even if Axis.Zoom() throws 5. Relax perf test threshold from 100ms to 1000ms to prevent flaky failures on slow CI runners 6. Add ArgumentOutOfRangeException guards on startIndex/endIndex in MinMaxDownsampler.Downsample sub-range overload Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tylerkron
force-pushed
the
claude/wonderful-yonath
branch
from
df81c9e to
a90e38f
Compare
Session 2 (18M samples, 32 channels) was taking ~30s to load because the entire dataset was read from SQLite before showing anything. Phase 1 (<1s): Get channel metadata from the first timestamp (6ms via composite index), load first 100K samples (16ms via index scan), and display immediately. The user sees data in under a second. Phase 2 (background): Stream remaining samples up to the 10M cap, then refresh the minimap and main plot with full-fidelity data. Key insight: SQLite's composite index on (LoggingSessionID, TimestampTicks) makes LIMIT queries nearly instant, but DISTINCT/COUNT/full scans over 18M rows are inherently slow (5-15s). By showing partial data first, perceived load time drops from 30s to <1s. Also extracted helper methods (PrepareMinimapData, SetupUiCollections, SetupMinimapSeries) to reduce duplication between phases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 2 was streaming all 10M+ rows sequentially (~30s). Now uses targeted index seeks: divides the time range into 3000 segments and seeks to each segment boundary via the composite index. Each seek reads one batch of interleaved channel data (~32 rows). Result: ~96K rows covering the full time range in ~1-3 seconds, regardless of total dataset size (18M, 100M, doesn't matter). Benchmark on 18M-sample session: - Before: ~30s (sequential scan of 10M rows through EF Core) - After: ~1-3s (3000 index seeks × 32 rows via raw ADO.NET) Uses raw ADO.NET with a prepared statement for minimal per-query overhead instead of EF Core materialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the user zooms into a narrow time window, the sampled in-memory data (~3000 points/channel) becomes too sparse to show the true waveform. Now detects this condition and fetches full-resolution data directly from the database for just the visible window. How it works: - UpdateMainPlotViewport() checks if the visible range of sampled data has fewer points than MAIN_PLOT_BUCKET_COUNT (2000) - If so, FetchViewportDataFromDb() queries the DB using the composite index: WHERE TimestampTicks BETWEEN @min AND @max - The fetched data is downsampled if needed and displayed - Falls back to in-memory data when zoomed out (sufficient density) Performance: a 1-second window at 1000Hz × 32 channels = ~32K rows, which takes ~16ms to read via the composite index. Stays well within the 60fps frame budget. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
During minimap drag and main plot pan/zoom, use only in-memory sampled data for instant viewport updates. Full-resolution DB queries are deferred until interaction ends (mouse up) or settles (200ms idle), ensuring smooth 60fps responsiveness even on large datasets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…anup
- Sparse check now examines ALL channels instead of just the first one
in dictionary iteration order; breaks only when a sparse channel is found
- Replace hardcoded PlotModel.Axes[0]/[2] with key-based lookups ("Time",
"Analog") in zoom commands for robustness against axis reordering
- Simplify duplicated if/else branches in MinimapInteractionController
OnMouseUp — both paths were calling the same methods
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the synchronous range-scan query in FetchViewportDataFromDb with sampled index seeks (same technique as LoadSampledData), capping reads at ~4000 * channelCount rows regardless of window size. Run the DB work on a background thread with CancellationToken support so the UI stays responsive and cursors update normally during the fetch. Add a thin indeterminate ProgressBar (2px, brand blue) between the main plot and minimap that appears while IsRefiningData is true — a subtle visual cue that higher-fidelity data is loading without any text or modal overlay. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ClearPlot now resets all axes (time, analog, digital) so selecting a new logging session starts at the full data range instead of keeping the previous session's zoom level. Also cancels any in-flight DB fetch and stops the settle timer on session switch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… loading ADR 001: Updated to document the hybrid approach — on-demand DB queries were partially adopted (async sampled seeks on settle) rather than fully rejected. Updated "How it works" to cover two-phase loading and drag/settle distinction. Added known issues (thread safety, consumer shutdown) to follow-up work. CLAUDE.md: Added three new gotchas — async DB fetch lifecycle, drag vs settle pattern, and session-switching axis reset. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
/agentic_review |
Contributor
|
Persistent review updated to latest commit f714fd7 |
Comment on lines
+651
to
+726
| private void LoadSampledData(int sessionId, int channelCount) | ||
| { | ||
| using var context = _loggingContext.CreateDbContext(); | ||
| var connection = context.Database.GetDbConnection(); | ||
| connection.Open(); | ||
|
|
||
| // Get time bounds via index (instant) | ||
| long minTicks, maxTicks; | ||
| using (var boundsCmd = connection.CreateCommand()) | ||
| { | ||
| boundsCmd.CommandText = @" | ||
| SELECT MIN(TimestampTicks), MAX(TimestampTicks) | ||
| FROM Samples | ||
| WHERE LoggingSessionID = @id"; | ||
| var idParam = boundsCmd.CreateParameter(); | ||
| idParam.ParameterName = "@id"; | ||
| idParam.Value = sessionId; | ||
| boundsCmd.Parameters.Add(idParam); | ||
|
|
||
| using var reader = boundsCmd.ExecuteReader(); | ||
| if (!reader.Read() || reader.IsDBNull(0)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| minTicks = reader.GetInt64(0); | ||
| maxTicks = reader.GetInt64(1); | ||
| } | ||
|
|
||
| if (minTicks >= maxTicks) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _firstTime = new DateTime(minTicks); | ||
| var tickStep = (maxTicks - minTicks) / SAMPLED_POINTS_PER_CHANNEL; | ||
| // Read at least channelCount rows per seek to get one sample per channel | ||
| var batchSize = Math.Max(channelCount * 2, 100); | ||
|
|
||
| // Prepared statement for repeated seeks | ||
| using var seekCmd = connection.CreateCommand(); | ||
| seekCmd.CommandText = @" | ||
| SELECT ChannelName, DeviceSerialNo, TimestampTicks, Value | ||
| FROM Samples | ||
| WHERE LoggingSessionID = @id AND TimestampTicks >= @t | ||
| ORDER BY TimestampTicks | ||
| LIMIT @limit"; | ||
|
|
||
| var seekIdParam = seekCmd.CreateParameter(); | ||
| seekIdParam.ParameterName = "@id"; | ||
| seekIdParam.Value = sessionId; | ||
| seekCmd.Parameters.Add(seekIdParam); | ||
|
|
||
| var seekTParam = seekCmd.CreateParameter(); | ||
| seekTParam.ParameterName = "@t"; | ||
| seekTParam.Value = minTicks; | ||
| seekCmd.Parameters.Add(seekTParam); | ||
|
|
||
| var seekLimitParam = seekCmd.CreateParameter(); | ||
| seekLimitParam.ParameterName = "@limit"; | ||
| seekLimitParam.Value = batchSize; | ||
| seekCmd.Parameters.Add(seekLimitParam); | ||
|
|
||
| seekCmd.Prepare(); | ||
|
|
||
| // Track which timestamps we've already added to avoid duplicates | ||
| // from overlapping batches | ||
| var lastAddedTimestamp = new Dictionary<(string, string), long>(); | ||
|
|
||
| for (var i = 0; i < SAMPLED_POINTS_PER_CHANNEL; i++) | ||
| { | ||
| var seekTimestamp = minTicks + i * tickStep; | ||
| seekTParam.Value = seekTimestamp; | ||
|
|
||
| using var reader = seekCmd.ExecuteReader(); | ||
| while (reader.Read()) |
Contributor
There was a problem hiding this comment.
1. executereader() used synchronously 📘 Rule violation ➹ Performance
New database access code uses synchronous EF/SQLite operations (e.g., Count(), ToList(), ExecuteReader()) instead of async APIs. This violates the requirement to use async DB methods and can reduce responsiveness and scalability even when run off the UI thread.
Agent Prompt
## Issue description
Database operations introduced/modified in this PR use synchronous EF/SQLite calls (`Count()`, `ToList()`, `ExecuteReader()`, `Open()`), violating the requirement to use async DB APIs.
## Issue Context
Even if these calls often run on background threads, the compliance rule requires async methods for DB access, and async enables cancellation tokens to be respected more consistently.
## Fix Focus Areas
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[510-516]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[542-542]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[651-750]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[1173-1248]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Disagree. All ExecuteReader() calls in LoadSampledData and FetchViewportDataFromDb run on background threads (DisplayLoggingSession runs on a BackgroundWorker; FetchViewportDataFromDb runs via Task.Run). SQLite is file-backed I/O, not network I/O — async APIs don't provide meaningful benefit and add complexity. The CancellationToken is already checked between seek iterations for responsive cancellation.
Comment on lines
+1045
to
+1065
| foreach (var kvp in _allSessionPoints) | ||
| { | ||
| if (kvp.Value.Count == 0) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Only check channels that are actually sampled (not full datasets) | ||
| if (kvp.Value.Count < SAMPLED_POINTS_PER_CHANNEL / 2) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var (si, ei) = MinMaxDownsampler.FindVisibleRange(kvp.Value, visibleMin, visibleMax); | ||
| var sampledVisible = ei - si; | ||
| if (sampledVisible < MAIN_PLOT_BUCKET_COUNT) | ||
| { | ||
| needsDbFetch = true; | ||
| break; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
4. Cross-thread points race 🐞 Bug ☼ Reliability
DatabaseLogger populates and clears _allSessionPoints on a BackgroundWorker thread while UI-thread DispatcherTimer callbacks iterate and index into the same dictionary/lists, which can throw (InvalidOperationException/ArgumentOutOfRangeException) during pan/zoom or minimap drag while loading.
Agent Prompt
### Issue description
`_allSessionPoints` (dictionary + per-channel `List<DataPoint>`) is written from the BackgroundWorker thread during `DisplayLoggingSession`/`LoadSampledData`, while UI-thread timers (`OnViewportThrottleTick` → `UpdateMainPlotViewport`/`UpdateSeriesFromMemory`) iterate and index into the same structures. This can crash or corrupt viewport updates.
### Issue Context
Session load occurs off the UI thread, but viewport updates are driven by `DispatcherTimer` on the UI thread and are always running.
### Fix Focus Areas
- Daqifi.Desktop/ViewModels/DaqifiViewModel.cs[1202-1234]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[244-268]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[468-615]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[915-946]
- Daqifi.Desktop/Loggers/DatabaseLogger.cs[1040-1120]
### Suggested fix approach
Choose one:
1) **Single-thread ownership**: build all point data in local data structures on the background thread, then `Dispatcher.Invoke` once to swap `_allSessionPoints` references (or replace per-channel lists) on the UI thread.
2) **Locking**: introduce a private lock (e.g., `_sessionPointsLock`) and lock around *all* reads/writes/iterations of `_allSessionPoints` and the contained lists (including `PrepareMinimapData`, `LoadSampledData`, `UpdateMainPlotViewport`, `ResetZoom`).
3) Temporarily **pause viewport timers** during session load/phase2 reload and resume after the in-memory data is stable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Acknowledged — deferred. This is a pre-existing architectural issue (the consumer thread and session loading both predate this PR). We've documented it in ADR 001 follow-up work and noted the suggested fix approaches (single-thread ownership via Dispatcher.Invoke swap, or explicit locking). In practice, Phase 2 loading completes within ~1-3s and the viewport timer check (_viewportDirty) limits the race window, but a proper fix is warranted in a dedicated PR.
…dispose - Cancel in-flight DB fetch when viewport changes to prevent stale data from overwriting the current view (Qodo #2.6) - Ensure LoadSampledData seeks at maxTicks on the final iteration so the session tail is always included in sampled data (Qodo #2.5) - Replace MinimapPlotModel.ResetAllAxes() with explicit axis.Zoom() from source data bounds to avoid incorrect auto-range (Qodo #2.2) - Unsubscribe timeAxis.AxisChanged in Dispose() to prevent leaks and duplicate callbacks (Qodo #2.3) - Add tickStep guard (Math.Max(1, ...)) for tiny time ranges Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📊 Code Coverage ReportSummarySummary
CoverageDAQiFi - 16.8%
Daqifi.Desktop.Common - 30.8%
Daqifi.Desktop.IO - 100%
Coverage report generated by ReportGenerator • View full report in build artifacts |
This was referenced Apr 13, 2026
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
1 participant
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.
Summary
List<DataPoint>per series during interaction instead of allocating ~960 new lists/sec.Take()to avoid materializing excess data(LoggingSessionID, TimestampTicks)for faster ordered session queriesInvalidatePlot(true)to force OxyPlot to re-read changed ItemsSource after viewport updatesSupersedes PR #457 — global LTTB decimation conflicts with the minimap (zooming into a 1-min slice of 24h would show ~3 points). This viewport-aware approach gives full detail when zoomed in.
Performance budget (16 channels × 1M pts/channel)
Files changed
MinMaxDownsampler.cs— addedFindVisibleRange()binary search + sub-rangeDownsample()overloadDatabaseLogger.cs— viewport-aware downsampling, throttled updates, guard flag, cached lists, removed 1M capMinimapInteractionController.cs— 60fps throttled rendering, guard flag integration,InvalidatePlot(true)fixLoggingContext.cs— composite DB indexMinMaxDownsamplerTests.cs— 11 unit tests covering downsampling, sub-range ops, binary search, and perf benchmarkTest plan
dotnet build— 0 errors)🤖 Generated with Claude Code