Defer cell loads for large S-57 exchange sets (issue #458)#459
Open
philliphoff wants to merge 10 commits into
Open
Defer cell loads for large S-57 exchange sets (issue #458)#459philliphoff wants to merge 10 commits into
philliphoff wants to merge 10 commits into
Conversation
Opening a large S-57 exchange set (e.g. 7,184 cells) previously loaded every cell eagerly, freezing the UI thread and exhausting memory (~33 GB, near-OOM). This introduces viewport-driven lazy loading. Above a threshold (default 50 cells) every cell is registered but its bytes are deferred: the Datasets panel lists cells immediately (dimmed, with extent outlines) and the coordinator loads only the cells that are in view at a relevant scale, through a bounded-concurrency gate, with LRU eviction beyond a retention budget. New decision core (unit-tested, pure): CellUsageBand, LazyCellGate, LruEvictionPolicy; stateful ExchangeSetLazyLoadCoordinator bridges the viewport notifier to gated loads/evictions. Registration is batched via DatasetsViewModel.AddRangeFromExchangeSet (backed by BulkObservableCollection) so thousands of cells register with a single collection notification instead of one per cell. Entries are born deferred so the coordinator's IsDeferred re-assertion is a no-op, avoiding an O(N^2) extent/grouping rebuild storm. Measured on the real 7,184-cell set: open dropped from ~32-139 s (UI-blocked) to ~3 s with the UI responsive throughout (concurrent set_viewport calls returned in 0.05 s), peak RSS ~3.9 GB, and only ~16-27 cells resident instead of 7,184. Follow-up (noted in #458): extend deferral to the S-100 CATALOG.XML and loose-cell-folder open paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
Contributor
Performance Gate✅ PASSED — no regressions. Threshold: 10.0%, MAD multiplier (k): 3.0, retry-zone mult: 2.0× Scenario summary
exchange-set-openIteration statistics
Spans (sum of all iterations)
Metrics
s101-portray-coldIteration statistics
s101-portray-warmIteration statistics
s101-real-coldIteration statistics
s101-real-warmIteration statistics
s101-render-warmIteration statistics
s102-coverageIteration statistics
s102-coverage-openIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-largeIteration statistics
s102-real-warmIteration statistics
s111-real-warmIteration statistics
s124-vectorIteration statistics
s201-vectorIteration statistics
Generated by EncDotNet.S100.PerfReport gate command |
There was a problem hiding this comment.
Pull request overview
This PR introduces viewport-driven lazy loading for very large S-57 exchange sets in the Avalonia viewer to avoid UI freezes and unbounded memory growth when opening thousands of cells (issue #458).
Changes:
- Add a lazy-loading coordinator (viewport intersection + usage-band scale gating) with bounded concurrency and LRU-based eviction.
- Register large exchange sets in bulk (single collection notification) and mark entries “deferred” up front, with catalogue footprints used for extent outlines before bytes are loaded.
- Extend viewer models/services to carry catalogue footprints and a Mercator resolution snapshot for scale calculations.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/EncDotNet.S100.Viewer.Tests/LruEvictionPolicyTests.cs | Adds unit tests for the LRU eviction bookkeeping. |
| tests/EncDotNet.S100.Viewer.Tests/LazyCellGateTests.cs | Adds unit tests for usage-band parsing, viewport intersection, and scale gating logic. |
| tests/EncDotNet.S100.Viewer.Tests/ExchangeSetLazyLoadCoordinatorTests.cs | Adds coordinator tests (load/in-view, defer/out-of-view, eviction). |
| tests/EncDotNet.S100.Viewer.Tests/DatasetsViewModelAddRangeTests.cs | Tests bulk registration semantics (born-deferred, single Reset notification, no-op deferred reassert). |
| src/EncDotNet.S100.Viewer/ViewModels/ExchangeSetCellRegistration.cs | New registration descriptor used by bulk exchange-set entry creation. |
| src/EncDotNet.S100.Viewer/ViewModels/DatasetsViewModel.cs | Adds GeographicBounds, UsageBand, IsDeferred; switches Entries to BulkObservableCollection; adds AddRangeFromExchangeSet. |
| src/EncDotNet.S100.Viewer/ViewModels/BulkObservableCollection.cs | Implements InsertRange raising a single Reset notification. |
| src/EncDotNet.S100.Viewer/Services/MapViewportSnapshot.cs | Adds Mercator resolution to support scale denominator computation. |
| src/EncDotNet.S100.Viewer/Services/MapViewportNotifier.cs | Populates Mercator resolution in published snapshots. |
| src/EncDotNet.S100.Viewer/Services/LazyLoading/LruEvictionPolicy.cs | New pure LRU eviction policy (key-order tracking + protected keys). |
| src/EncDotNet.S100.Viewer/Services/LazyLoading/LazyCellGate.cs | New pure gating logic (viewport intersection + usage-band eligibility + scale conversion). |
| src/EncDotNet.S100.Viewer/Services/LazyLoading/ExchangeSetLazyLoadCoordinator.cs | New stateful coordinator (debounce, gating, bounded load pump, LRU eviction). |
| src/EncDotNet.S100.Viewer/Services/LazyLoading/CellUsageBand.cs | New band parser (ENC usage band from cell name). |
| src/EncDotNet.S100.Viewer/Services/IDatasetLoaderService.cs | Adds UnloadEntry default member for eviction-style unload. |
| src/EncDotNet.S100.Viewer/Services/ExchangeSetService.cs | Adds lazy-load path for large S-57 sets: bulk register + coordinator registration + footprint propagation. |
| src/EncDotNet.S100.Viewer/Services/DatasetLoaderService.cs | Implements UnloadEntry that unloads layers/processors but keeps the entry registered and deferred. |
| src/EncDotNet.S100.Viewer/Services/DatasetExtentIndicatorController.cs | Draws catalogue footprint outlines for deferred entries; adds EPSG:4326→3857 projection helper. |
| src/EncDotNet.S100.Viewer/README.md | Documents the new lazy-loading behavior and relevant types. |
| src/EncDotNet.S100.Viewer/App.axaml.cs | Registers the coordinator in DI and wires load/unload delegates. |
Fixes four review findings on the S-57 lazy-loading PR (#458/#459): 1. Threading: marshal the debounced Evaluate() back to the UI thread via the viewport notifier's captured SynchronizationContext, so loader/ Mapsui/DatasetEntry mutations no longer run on the thread pool. 2. Lifecycle: PumpLoadAsync now detects Unregister()/Dispose() that raced its load and unwinds the finished load (unload) instead of marking a closed exchange set's cell loaded (no zombie layers). 3. Contract: IDatasetLoaderService.UnloadEntry's default member now honours its IsDeferred contract before removing; remarks clarified. 4. Messaging: large deferred opens use distinct "Exchange Set Ready / Registered N cell(s); loading on demand" strings instead of the "Loaded" toast; ExchangeSetOpenResult.Loaded documents its registered-count meaning on the deferred path. Adds a coordinator test pinning the abort-on-unregister behaviour. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
Second review pass on #459: - PumpLoadAsync now catches ObjectDisposedException from _gate.WaitAsync() (Dispose() disposed the semaphore while the load was queued) and returns, and guards _gate.Release() in the finally, so the fire-and-forget pump never surfaces an unobserved exception during teardown. On the disposed path it also skips the abort-unload so the map is not touched during shutdown. - LruEvictionPolicy<TKey> is constrained to a reference type and keyed by ReferenceEqualityComparer, making the documented by-identity tracking true regardless of TKey's equality overrides. Adds a coordinator test that disposes mid-load and asserts no throw. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
Third review pass on #459: - ExchangeSetLazyLoadCoordinator.Unregister now also removes the entry from _loadedEntries (the LRU mirror). Previously a closed cell left in the mirror could be re-Touch()'d back into the LRU by a later Evaluate(), keeping the coordinator acting on an unregistered cell. Added a regression test. - Removed the unused System.Linq using from LruEvictionPolicy. - Reworded LruEvictionPolicy remarks to not reference the view-model DatasetEntry type (the type is intentionally kept view-model-free). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
Fourth review pass on #459: DatasetLoaderService.UnloadEntry no longer clears the entry's _activeFlags entry. Eviction unloads bytes but keeps the DatasetEntry registered, so removing the flag silently reset a user-set inactive cell back to active (the GetActive default) when it reloaded. The flag is now only cleared on full RemoveEntry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
LazyCellGate.IntersectsViewport now splits seam-crossing longitude ranges (west > east) into two non-wrapping segments before testing overlap, so dateline-crossing cells/viewports are no longer treated as disjoint. DatasetExtentIndicatorController splits seam-crossing deferred footprints into up to two mercator boxes so their outlines still render (ToMercatorExtent previously returned null when maxX < minX). Adds LazyCellGate seam-crossing regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
The viewport-debounce continuation now no-ops unless its CTS is still the current _debounceCts, so a snapshot that completed Task.Delay just before a newer viewport change superseded it no longer runs a stale Evaluate(). The current CTS is disposed when its continuation fires (previously only superseded ones were disposed, leaking the last one). Adds a debounced-path regression test asserting rapid viewport changes coalesce to a single load of the latest snapshot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
…r stable DatasetLoaderService.UnloadEntry no longer removes the evicted cell from _entryOrder. RemoveEntryLayers already cleared its layers, and FlattenLayerOrder skips entries without layers, so the slot is inert while unloaded but preserved — the next reload is no longer treated as a first load (ReplaceLayers.isFirstLoad) and re-inserted at index 0, which had reshuffled cross-dataset paint order on every eviction/reload cycle. RemoveEntry still drops the entry (full close). Also names both arguments in an LruEvictionPolicy test call for clarity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
PumpLoadAsync now re-checks tracking under _lock right after acquiring the gate and before _loadAsync: a load queued behind the semaphore whose cell was Unregister()'d (or whose coordinator was Dispose()'d) while waiting is abandoned before the expensive parse/portray runs, instead of doing the work and unwinding afterward. DatasetsViewModel.AddRangeFromExchangeSet now validates each registration's Source/RelativePath/ProductSpec up front for parity with AddFromExchangeSet, failing fast rather than creating an invalid DatasetEntry. Adds coordinator queued-abandon and view-model validation regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
ExchangeSetLazyLoadCoordinator.Register now skips entries already tracked in any state (deferred/loading/loaded), matching its documented idempotency: re-registering a since-loaded cell no longer re-marks it IsDeferred (which would dim it and draw a deferred outline while its layers are still on the map). DatasetExtentIndicatorController's deferred fast-path now honors IsVisible, so hiding a deferred cell removes its outline just like a hidden loaded dataset. Adds coordinator idempotency and deferred-visibility regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa540bcb-21b1-46ad-9697-e6cffc627472
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.
Problem
Opening a large S-57 exchange set (e.g. the 7,184-cell set) loaded every cell eagerly, freezing the UI thread and exhausting memory (~33 GB, near-OOM). Fixes #458.
Approach — viewport-driven lazy loading
Above a threshold (default 50 cells), every cell is registered but its bytes are deferred:
CellUsageBand,LazyCellGate,LruEvictionPolicy; statefulExchangeSetLazyLoadCoordinatorbridges the viewport notifier to gated loads/evictions.DatasetsViewModel.AddRangeFromExchangeSet(backed byBulkObservableCollection) so thousands of cells register with a single collection notification. Entries are born deferred, so the coordinator'sIsDeferredre-assertion is a guarded no-op — this removed a hidden O(N²) extent/grouping rebuild that alone cost ~29s on the real set.Measured (real 7,184-cell set)
set_viewportreturned in 0.05 s)Tests
AddRangeFromExchangeSettests (born-deferred, single Reset, no-op re-assert) plus lazy-core tests (LazyCellGate,LruEvictionPolicy, coordinator).Scope / follow-up (deferred — see #458)
Kept intentionally scoped to S-57 exchange sets (the demonstrated pain). Noted for a follow-up generalization pass:
DatasetLoaderService(no longer a bottleneck now that resident cells are bounded).CATALOG.XMLopen path (BoundingBoxalready available).S-100 sets are typically < 50 cells and stay eager for now.