Skip to content

NavigateTo copies the whole history on every call, even with no undo provider; with an undo provider each retained action keeps two full snapshots (O(n) per navigation, ~125 MB at 50k entries) #72

Description

@matt-edmondson

What's wrong

Navigation<T>.NavigateTo snapshots the whole item list on every call, even when no undo provider is configured and the snapshot is thrown away:

public void NavigateTo(T item)
{
Ensure.NotNull(item);
T? previousItem = Current;
// Capture state before navigation for undo
int beforeIndex = _currentIndex;
List<T> beforeItems = [.. _items];
// Remove any forward history when navigating to a new item
if (_currentIndex < _items.Count - 1)
{
_items.RemoveRange(_currentIndex + 1, _items.Count - _currentIndex - 1);
}
_items.Add(item);
_currentIndex = _items.Count - 1;
// Create undoable action if undo/redo provider is available
if (undoRedoProvider != null)
{
NavigateToAction<T> action = new(this, beforeIndex, beforeItems, _currentIndex, [.. _items]);
undoRedoProvider.RegisterAction(action, $"Navigate to {item.DisplayName}");
}
OnNavigationChanged(NavigationType.NavigateTo, previousItem, Current);

With an undo provider it copies the list again, and NavigateToAction then copies both lists once more:

internal sealed class NavigateToAction<T>(Navigation<T> navigation, int beforeIndex, List<T> beforeItems, int afterIndex, List<T> afterItems) : IUndoableAction where T : class, INavigationItem
{
private readonly Navigation<T> _navigation = Ensure.NotNull(navigation);
private readonly List<T> _beforeItems = [.. beforeItems];
private readonly List<T> _afterItems = [.. afterItems];
/// <inheritdoc />
public string Description => afterItems.Count > 0 ? $"Navigate to {afterItems[afterIndex].DisplayName}" : "Navigate";
/// <inheritdoc />
public void Execute() => _navigation.RestoreState(_afterItems, afterIndex);
/// <inheritdoc />
public void Undo() => _navigation.RestoreState(_beforeItems, beforeIndex);

The result:

  • Every navigation costs O(history).
  • Each of the up to maxHistorySize retained undo actions holds two full copies of the history.
  • The navigation list itself has no size cap.

This contradicts the docs:

  • docs/Architecture.md promises "O(1) navigation operations" and says "Configurable history limits prevent memory leaks".
  • docs/Design-Decisions.md ("History Limits … Prevents unbounded memory growth") doesn't hold, because the undo limit caps the number of snapshots, not their size.

Repro (net10.0, Release): time for 1000 further NavigateTo calls at a given history size

history=  1000 undo=False   8.0 ms  heap=0 MB
history=  1000 undo=True   18.9 ms  heap=3 MB
history= 10000 undo=False  31.9 ms  heap=2 MB
history= 10000 undo=True  648.5 ms  heap=24 MB
history= 50000 undo=False 167.7 ms  heap=10 MB
history= 50000 undo=True  677.0 ms  heap=125 MB

Suggested fix / acceptance criteria

  • Take no snapshot when undoRedoProvider is null.
  • Have NavigateToAction record only the delta instead of full before/after lists: the truncated forward items, the pushed item, and the before/after indices.
  • Optionally add a maxHistorySize to Navigation<T> (or its factory) that drops the oldest entries, so the history itself is bounded as the docs claim.
  • Tests:
    • Undo and redo after NavigateTo, including after forward history was truncated, restore identical stacks.
    • NavigateTo time stays flat as the history grows.
  • Coordinate with Undo after GoBack, Clear or LoadStateAsync drops the newest page and jumps to an unrelated state #55, which changes what undo must restore after GoBack, Clear and Load.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions