Skip to content

fix(performance): bound cached data - #2379

Merged
datlechin merged 2 commits into
TableProApp:mainfrom
sophiathedev:fix/result-memory-metadata
Aug 22, 2026
Merged

fix(performance): bound cached data#2379
datlechin merged 2 commits into
TableProApp:mainfrom
sophiathedev:fix/result-memory-metadata

Conversation

@sophiathedev

@sophiathedev sophiathedev commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • release registry and unpinned result rows when a reloadable table tab is evicted
  • preserve query, pinned, edited, selected, and in-flight results
  • track row display cache cost independently of mutable boxes
  • skip eager all-column loading when a schema exceeds the 50-table cache

Visual

flowchart TD
    A["Inactive tab"] --> B{"Reloadable table tab?"}
    B -- "no" --> C["keep all data"]
    B -- "yes" --> D{"Pinned, edited, loading, or executing?"}
    D -- "yes" --> C
    D -- "no" --> E["clear registry rows and ID index"]
    D -- "no" --> F["clear unpinned result rows and ID index"]
    E --> G["reload on next activation"]
    F --> G
Loading

Evidence

Regression case Before After / test proof
Tab eviction result snapshots and ID index retained the payload 10 to 0 rows in both owners; row 0 lookup returns nil
Display cache mutable 1 to 9 byte update was undercounted 9 + 2 exceeds the 10 byte limit; oldest entry is evicted
Schema columns whole schema was bulk-fetched, then only 50 tables were retained 50 tables gives 1 bulk fetch; 51 gives 0 bulk fetches and 1 lazy fetch

Tab eviction

  • eviction is limited to reloadable background table tabs
  • selected, query, pinned, edited, loading, fetch-all, and executing tabs remain unchanged
  • rows and ID indexes are released from both the registry and every unpinned result snapshot

Display cache

  • each cache entry records its accepted cost independently from the mutable box
  • clear and refill restores the correct recorded cost

Schema columns

  • schemas within the 50-table cache capacity keep eager loading
  • larger schemas skip bulk loading and fetch only requested tables

Verification

  • 115 focused tests passed, 0 failures
  • covered eviction, row ownership, display cache, schema provider, lazy reload, pinning, and result handoff suites
  • SwiftLint strict passed with 0 violations on all changed Swift files
  • git diff --check passed

- release both row owners during safe table-tab eviction
- track display-cache costs independently of mutable boxes
- skip eager column loads beyond cache capacity
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@datlechin

Copy link
Copy Markdown
Member

Reviewed this and pushed the fixes onto the branch rather than leaving a list. The direction is right: a ResultSet's TableRows and the registry's buffer are the same struct value, so they share COW storage and clearing only one frees nothing. That is a real leak and this is the right place to fix it.

What I changed, and why.

The eager-load skip counted the wrong tables. startEagerColumnLoad gated on tables.count, but SchemaRefreshService.syncAutocompleteProvider fills that list from SchemaService.allLoadedTables, which is the union of every schema the sidebar has expanded. The fetch it gates, fetchAllColumns(), covers one schema (PluginDriverAdapter passes pluginDriver.currentSchema). On PostgreSQL a ten-table public lost its preload permanently as soon as eight other schemas were open. The provider now records the schema it will fetch and counts only that schema's tables, falling back to the whole list for a flat engine that attributes no schema.

The threshold was the LRU capacity. Reusing maxCachedTables (50) as the eligibility bound put the cliff at 51 tables, where a bulk fetch is still one cheap query and the preload covers most of the schema. Columns are small next to row data, so the cap is now 300 and it is still one number: "only bulk-fetch a schema we can keep all of" is the rule that makes the skip correct.

isExecuting does not mean "this tab is idle". It is entries[tabId] != nil. Fetch All deliberately takes no claim (claiming mints a content epoch and would discard its own rows) and registers through beginUnclaimedWork, so the guard was carried entirely by the separate isLoadingMore flag. Added TabExecutionRegistry.isBusy(_:), the per-tab counterpart of isAnyExecuting, and the eviction gate reads that.

The gate was written out three times. evictReloadableTableRows and evictInactiveTabs each restated canAutoLoadTableTab, and the copies already disagreed over lastExecutedAt != nil. Those two answers have to agree or a tab is evicted with no route back to its rows. One canEvictReloadableTableRows(_:) now serves both, and it tests the query with contains(where:) instead of allocating a trimmed copy on a hot path.

The display cache took the cost from its caller. All three sites computed it identically from the box they were passing, so nothing checked they agreed and a wrong number would have skewed totalCost for the life of the cache. setBox(_:forID:) computes it from the box now; the per-entry record you added stays, because it is what makes the accounting symmetric.

Two things found while tracing this that are pre-existing but sit in the same subsystem, so they ride along:

  • TabSessionRegistry.updateTableRows cleared isEvicted unconditionally. Phase-2 metadata (applyPhase2Metadata, applyEnumValues) lands at .utility priority long after the load, gated only by resultStillActive, which eviction does not change. An evicted tab whose enum values arrived late ended up with zero rows and isEvicted == false, which canAutoLoadTableTab reads as already loaded, and eviction could not re-mark it because evict guards on !rows.isEmpty. The grid stayed empty until an explicit refresh. A mutation that leaves the buffer empty no longer clears the flag.
  • reloadVisibleRowsAndStates empties the whole display cache but was the only whole-cache invalidation not restarting the prewarm, so undo, redo and a theme change left every visible cell reformatting on the scroll hot path.

Three review findings I looked at and did not act on:

  • Dropping the !hasPinnedResults guard so the where !resultSet.isPinned filter does something. ResultTabBarPolicy.canPin requires tabType == .query, both pin call sites are gated by it (MainEditorContentView.swift:772, MainSplitViewController+MenuValidation.swift:208), and the one place tabType mutates (QueryTabManager.swift:418) clears display.resultSets at :428. A table tab cannot hold a pinned result, and if it could, that result is the active one sharing the buffer, so evicting would free nothing and cost a reload.
  • Emptying the active ResultSet breaking a zero-cost restore. There is no such path for a table tab: resolvedTableRows reads only the registry, and every result-strip re-point is .query-only while eviction is .table-only. The snapshot was pure duplication.
  • The schema.table vs bare-name cache-key mismatch. Real, but it predates this branch and fixing it needs the fetched schema plumbed through ColumnMetadataSource.fetchAllColumns. Left for its own change.

Tests: added coverage for evictInactiveTabs (the budget path that actually fires on every tab switch and had none), the blank-query and unclaimed-work guards, late metadata not resurrecting an evicted tab, the eager-load scope, and the cache deriving its own cost. eagerLoadRespectsMaxCachedTables was passing vacuously after the skip (60 tables gave 0 items and asserted 0 <= 50); it now asserts what actually happens.

@datlechin
datlechin force-pushed the fix/result-memory-metadata branch from 6adeecb to e2202b3 Compare August 22, 2026 08:46
@datlechin
datlechin merged commit 6a9ff3e into TableProApp:main Aug 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants