perf(sidebar): scope schema state per connection so large table lists stay responsive#1901
perf(sidebar): scope schema state per connection so large table lists stay responsive#1901datlechin wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca7b2bcd7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| private func bumpGeneration(_ connectionId: UUID) { | ||
| generations[connectionId, default: 0] &+= 1 | ||
| SidebarPerfSignpost.recordEvent("SchemaService.mutation", connectionId: connectionId) |
There was a problem hiding this comment.
Direct invalidate(connectionId:) calls on disconnect/reconnect/database switches remove the observable schema dictionaries without calling bumpGeneration; because the event is only emitted here, those invalidations can still cause the cross-window observation invalidations this probe is meant to diagnose while leaving no SchemaService.mutation entry. Please emit the signpost in invalidate as well, or move the event to a wrapper that covers both bump and reset mutations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8840b70be8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| internal static let signposter = OSSignposter(subsystem: subsystem, category: "SidebarPerf") | ||
| internal static let logger = Logger(subsystem: subsystem, category: "SidebarPerf") | ||
|
|
||
| private static let rowConstructions = OSAllocatedUnfairLock(initialState: 0) |
There was a problem hiding this comment.
This counter is process-wide, but recordBodyEvaluation drains it and tags the drained value with whichever connectionId is currently evaluating SidebarView.body. With two open windows/connections, rows constructed for connection A can be consumed by connection B's next body event, so the connection-tagged signposts become misleading exactly in the cross-window invalidation scenario this probe is meant to diagnose. Keep row counts scoped by connection or by an explicit body pass instead of using one global counter.
Useful? React with 👍 / 👎.
datlechin
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70ad479e08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isPendingTruncate: context.pendingTruncates.contains(ref.table.name), | ||
| isPendingDelete: context.pendingDeletes.contains(ref.table.name) | ||
| isPendingDelete: context.pendingDeletes.contains(ref.table.name), | ||
| showComment: context.showComment |
There was a problem hiding this comment.
When the database-tree layout is visible and the user toggles General > Object Comments, existing outline rows keep the old value because this showComment flag is captured in DatabaseTreeRowContext during AppKit cell rebuilds. I checked the DatabaseTreeOutlineView/DatabaseTreeOutlineCoordinator.update(from:) path, and that representable does not refresh on AppSettingsManager.general.showObjectComments; before this change each TableRow read AppSettingsManager.shared directly, so its NSHostingView observed the setting. This leaves comments stale until some unrelated tree reload happens.
Useful? React with 👍 / 👎.
094e61e to
0e231fe
Compare
1 participant
Problem
The sidebar and the app lag on connections with many tables. Confirmed with
os_signpostinstrumentation against a 2000-table SQLite schema (log stream --predicate 'subsystem == "com.TablePro" && category == "SidebarPerf"'):TableRowvalues for a 2,000-table list, roughly 4x the rows. Loading the connection's stored procedures/functions rebuilt the entire table list, because every schema mutation bumped one shared generation that the table cache keyed on.SchemaService.mutationon one connection re-evaluated the sidebar body of a different connection's window.SchemaServiceandDatabaseTreeMetadataServicekept every connection's state in one@Observable, and Observation tracks per stored property, so any connection's load invalidated every window that had read that property, includingMainContentView.Per-keystroke body passes with unchanged results built 0 rows (~1ms) and were not the cause; the cost is the full-list rebuilds above.
Fix
Scope the two metadata services per connection, following the existing
SharedSidebarState/SidebarViewModelregistry pattern.SchemaConnectionState(new): one@Observableobject per connection holding that connection's schema state, with separatetablesRevisionandroutinesRevision.SchemaServicekeeps all its load and supersession logic and becomes a thin forwarder to the per-connection holder.SidebarViewModelkeys its table caches ontablesRevisionand its routine cache onroutinesRevision. A procedures/functions load now bumps onlyroutinesRevision, so the table list's cached array is unchanged and SwiftUI'sForEachskips rebuilding all table rows.DatabaseTreeConnectionState(new): the same split for the tree metadata service, so a background load on one connection no longer redrawsMainEditorContentViewand the tree in every other window.Set<String>instead of allocating aFavoriteTablesStorage.FavoriteEntryper row, and each row's "show comment" flag is passed down once instead of everyTableRowreadingAppSettingsManager.shared.Observation scoping was verified with a standalone probe before implementing: reading a per-connection object through the stateless facade keeps observation scoped to that object (mutating connection B fires 0 notifications for a connection-A reader; bumping
routinesRevisionfires 0 for atablesRevisionreader).What this does not change
Listand the native.sidebarsource-list appearance. The.listStyle(.sidebar)eager row construction is real but only material past ~2,000 tables, and it is the native look, so it stays.Instrumentation
Keeps a small permanent
SidebarPerfSignpost(OSSignposter + Logger, subsystemcom.TablePro, categorySidebarPerf): body evaluations, schema mutations tagged tables vs routines, and the tree refresh interval. Free when nothing is tracing. Verify before/after with:After the fix, a routines load emits
SchemaState.routinesand no longer rebuilds the table rows, and a load on one connection no longer emitsbody SidebarViewfor another connection.Tests
SchemaConnectionStateTests: registry identity and eviction; cross-connection isolation; a routines load bumps onlyroutinesRevision(and a tables load onlytablesRevision);reset()preserves instance identity so bound views survive a reconnect.DatabaseTreeConnectionStateTests: the same isolation and lifecycle guarantees for the tree service.SchemaService,DatabaseTreeMetadataService, andSidebarViewModeltests keep their signatures and pass unchanged.Notes
.listStyle(.sidebar)magnitude.