Add scripted-measurement MCP tools and fix render_to_image PNG leak (perf-report follow-ups)#161
Merged
Merged
Conversation
Adds the first mutating MCP tool, distinguished from the existing read-only render_to_image. Accepts either a WGS-84 bbox (south/west/north/east) or a centre+zoom pair (centerLat/centerLon/ zoom) and drives the live viewer's navigator. Closes part of the tooling gap identified in the portrayal/render perf report (§7): without this, scripted measurement runs could not drive pan/zoom from outside the GUI, so warm 'viewport-change' scenarios were unmeasurable. Pattern (request/result records, IMapHost surface, MCP adapter, FakeMapHost test seam) can be reused for set_palette, set_time_step, and close_dataset in follow-up PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two more mutating MCP tools, alongside set_viewport. The viewer's palette (Day/Dusk/Night) and ECDIS display category (DisplayBase/ Standard/OtherInformation/All) can now be driven from scripted measurement runs. Introduces IRenderStateController + accessor as the indirection between MCP tools and SettingsViewModel / EcdisDisplayState. The controller marshals every setter through the Avalonia UI dispatcher, so off-thread MCP request handlers can drive UI-affine state without races. Mirrors the IMapHostAccessor late-binding pattern. Extracts a small ToolErrorPayload helper so every adapter emits the same failure shape — it was getting copy-pasted. Closes part of the tooling gap from the portrayal/render perf report (§7); palette-switch and display-category-change scenarios were not externally drivable before this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drives the viewer's GlobalTimeService to a specific sample for time-aware datasets (S-104 / S-111 / S-411). Accepts either a 0-based index or an ISO-8601 timestamp (snapped to the nearest sample), mirroring the --time-step CLI flag but applicable mid-session. Internal dispatcher seam (Func<Action, Task>) lets the tool be unit-tested without a running Avalonia application; production construction wires it to Dispatcher.UIThread.InvokeAsync so the notify path runs where bindings expect. Closes part of the tooling gap from the perf report (§7); time-step churn scenarios for S-111 / S-104 were not externally drivable before this. Final tool in the 'set_*' family for PR-A2..A4 of the optimisation plan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The MCP render_to_image clone-Map path was constructing a Mapsui Map on every call and letting it go to the GC unsdisposed. Mapsui.Map implements IDisposable, and its un-run dispose path leaves LayerCollection / property-changed plumbing rooted indefinitely. Over 150 sequential renders, perf-report Track-B measurements saw RSS climb from ~144 MB to ~1.46 GB — the per-call PNG buffer plus its associated SkiaSharp bitmaps could not be reclaimed. Fix: detach the live layers from the snapshot Map before disposing it (so the snapshot's dispose path only touches its own owned resources, never the live layer instances we don't own), then call Dispose explicitly via try/finally. Layer-collection teardown is best-effort — a teardown failure should never mask a render failure or crash the dispatcher. This was the highest-priority candidate from the perf report (§6 P1) and the plan called it out as 'must land before any other Track-B retention work' — without it, every gcdump comparison is contaminated by this leak. Validation belongs to the Track-B path (live viewer + dotnet-gcdump over 150 renders); MapsuiMapHost has no unit-test surface because its real implementation requires an Avalonia MapControl. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Spans (sum of all iterations)
Metrics
s101-portray-warmIteration statistics
Spans (sum of all iterations)
Metrics
s101-real-coldIteration statistics
s101-real-warmIteration statistics
s101-render-warmIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverageIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-openIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-largeIteration statistics
Spans (sum of all iterations)
Metrics
s102-real-warmIteration statistics
s111-real-warmIteration statistics
s124-vectorIteration statistics
Spans (sum of all iterations)
Metrics
s201-vectorIteration statistics
Spans (sum of all iterations)
Metrics
Generated by EncDotNet.S100.PerfReport gate command |
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.
Implements the highest-priority items from the perf-report optimisation plan: 3 new Viewer MCP tools that unblock scripted performance measurement, plus the P1 fix for the PNG-buffer / native-bitmap retention leak in
render_to_image.Commits (split for per-commit review)
2c3dd08set_viewportMCP tool — bbox or center+resolution; extendsIMapHostwithSetViewportToExtent/SetViewportToCenterAndResolution5855e97set_palette+set_display_categoryMCP tools; introducesIRenderStateControllerUI-thread marshaling abstractionb283f18set_time_stepMCP tool — accepts int index or ISO timestamp, snaps to nearest sampledbe48f3render_to_imagesnapshot-Mapretention leak (responsible for 144 MB → 1.46 GB RSS growth over 150 renders in the perf report)Why these four together
The 3 MCP tools (
set_viewport,set_palette/set_display_category,set_time_step) are the scripted-measurement primitives the perf-report's Track-A and Track-B harnesses need to drive deterministic warm-render scenarios. The P1 fix is inMapsuiMapHost.RenderCurrentViewToPngAsync— the body of the existingrender_to_imageMCP tool, which is itself one of those measurement primitives. Without P1, every Track-B retention measurement is contaminated by ~10 MB of leaked PNG buffer and native-bitmap memory per render call. The plan called this out as must land before any other Track-B retention work.P1 root cause + fix
Mapsui.MapimplementsIDisposable, but the snapshotMapconstructed per-call inRenderCurrentViewToPngAsyncwas let go to the GC un-disposed. ItsLayerCollectionand property-changed plumbing kept the snapshot rooted indefinitely, holding the per-call PNG buffer plus its associated SkiaSharp bitmaps.Fix: detach live layers (
snapshot.Layers.ClearAllGroups()) before disposing the snapshot, so the snapshot's dispose path only releases its own owned resources and never touches the live layer instances we share with the on-screenMap. Layer-collection teardown is wrapped in a best-efforttry/catchso a teardown failure cannot mask a render failure.Verification
MapsuiMapHostrequires a real AvaloniaMapControl. Validation for the leak fix belongs to the perf-report's Track-B path: rerun the 150-renderdotnet-gcdumpharness against this branch and expect the RSS ceiling to drop from ~1.46 GB to roughly 200 MB. Adding a headless Avalonia harness forMapsuiMapHostis a candidate follow-up tooling gap (it would also enable end-to-end testing of PR-A1's viewport setters).Docs
docs/mcp-server.mdandsrc/EncDotNet.S100.Viewer/README.mdupdated to describe the 4 new tools and the read-only / mutating tool convention.Not in this PR
The remaining 12 items from the optimisation plan (PR-A4
close_dataset, PR-A5, the 4 PerfRunner harness improvements, and P2–P7) are deliberately deferred — they all benefit from re-running the perf harness against the post-P1 baseline, and shipping them blind would violate the plan's own sequencing rule.