perf: replace state snapshots with DB proxy layer for saveDb performance#1233
perf: replace state snapshots with DB proxy layer for saveDb performance#1233kangjoseph90 wants to merge 1 commit intokwaroran:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves database save performance by replacing the snapshot-based change detection in saveDb() with a proxy-based event-driven approach. Instead of taking expensive snapshots of the entire database to detect changes, a Proxy layer now emits events whenever database properties are modified, enabling O(1) reactive change tracking.
Changes:
- Added proxy-based change detection layer with
createDatabaseProxy(),onDatabaseUpdate()API, andDatabaseUpdateInfointerface - Refactored
saveDb()to use event-driven callbacks fromonDatabaseUpdate()instead of$effectwith$state.snapshotoperations - Modified
setDatabaseLite()to wrap database objects in the new proxy layer
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| src/ts/storage/database.svelte.ts | Implements the proxy infrastructure including DatabaseUpdateInfo interface, listener management, proxy creation with caching, and path normalization logic |
| src/ts/globalApi.svelte.ts | Refactors saveDb() to replace $effect-based snapshot tracking with onDatabaseUpdate callback-based change detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| } | ||
|
|
||
| updateChangeTrackerForCharacter(DBState?.db?.characters?.[selIdState]) |
There was a problem hiding this comment.
The updateChangeTrackerForCharacter function may be called with an invalid character object that lacks a chaId, causing it to return early without updating the change tracker. However, the caller at line 423 (the catch-all case for non-character root keys) always calls this function with the current selected character, which could be undefined or invalid. In such cases, updates to other root-level database properties (like apiType, mainPrompt, etc.) would silently fail to trigger a save. Consider tracking these general updates separately or ensuring all database root properties are explicitly handled.
| updateChangeTrackerForCharacter(DBState?.db?.characters?.[selIdState]) | |
| const selectedChar = DBState?.db?.characters?.[selIdState] | |
| if (selectedChar && selectedChar.chaId) { | |
| updateChangeTrackerForCharacter(selectedChar) | |
| } |
| const index = typeof rawIndex === 'number' | ||
| ? rawIndex | ||
| : (typeof rawIndex === 'string' && /^\d+$/.test(rawIndex) ? Number(rawIndex) : selIdState) | ||
| const targetChar = DBState?.db?.characters?.[index ?? selIdState] |
There was a problem hiding this comment.
The index calculation logic unnecessarily complicates the code. The ternary operator 'index ?? selIdState' is redundant because if index is undefined (when rawIndex doesn't match the patterns), it already falls back to selIdState in the previous assignment. Using 'index ?? selIdState' here suggests potential confusion about when index could be null vs undefined vs 0 (which are all falsy). Consider simplifying to just use 'index' or clarify the intended behavior.
| const targetChar = DBState?.db?.characters?.[index ?? selIdState] | |
| const targetChar = DBState?.db?.characters?.[index] |
| emitDatabaseUpdate({ | ||
| path: [...path, normalizePathKey(obj, prop)], | ||
| value: undefined, | ||
| oldValue, | ||
| type: 'delete' | ||
| }) |
There was a problem hiding this comment.
The deleteProperty trap emits an update event even if the deletion fails. The event is emitted after deleteProperty is called but without checking if the result is true. If the deletion fails (e.g., for non-configurable properties), the event will incorrectly indicate that the property was deleted. Move the emitDatabaseUpdate call inside a conditional check for the result value.
| emitDatabaseUpdate({ | |
| path: [...path, normalizePathKey(obj, prop)], | |
| value: undefined, | |
| oldValue, | |
| type: 'delete' | |
| }) | |
| if (result) { | |
| emitDatabaseUpdate({ | |
| path: [...path, normalizePathKey(obj, prop)], | |
| value: undefined, | |
| oldValue, | |
| type: 'delete' | |
| }) | |
| } |
| set(obj, prop, value, receiver) { | ||
| // console.log('[DBProxy] Setting database property', [...path, normalizePathKey(obj, prop)].join('.')) | ||
| const oldValue = Reflect.get(obj, prop, receiver) | ||
| const result = Reflect.set(obj, prop, value, receiver) | ||
| if (oldValue !== value) { | ||
| emitDatabaseUpdate({ | ||
| path: [...path, normalizePathKey(obj, prop)], | ||
| value, | ||
| oldValue, | ||
| type: 'set' | ||
| }) | ||
| } | ||
| return result |
There was a problem hiding this comment.
The set trap emits an update event even if the set operation fails. The event is emitted after Reflect.set is called but without checking if the result is true. If the set fails (e.g., for read-only properties), the event will incorrectly indicate that the property was changed. Wrap the emitDatabaseUpdate call in a conditional check for the result value.
| const unsubscribeDb = onDatabaseUpdate((info) => { | ||
| // console.log('[DBProxy] Database updated at path:', info.path.join('.')) | ||
| const rootKey = info.path[0] | ||
|
|
||
| if (rootKey === 'botPresets' || rootKey === 'botPresetsId') { | ||
| changeTracker.botPreset = true | ||
| saveTimeoutExecute() | ||
| return | ||
| } | ||
|
|
||
| if (rootKey === 'modules') { | ||
| changeTracker.modules = true | ||
| saveTimeoutExecute() | ||
| return | ||
| } | ||
|
|
||
| if (rootKey === 'characters') { | ||
| const rawIndex = info.path[1] | ||
| const index = typeof rawIndex === 'number' | ||
| ? rawIndex | ||
| : (typeof rawIndex === 'string' && /^\d+$/.test(rawIndex) ? Number(rawIndex) : selIdState) | ||
| const targetChar = DBState?.db?.characters?.[index ?? selIdState] | ||
| const fallbackChar = info.oldValue && typeof info.oldValue === 'object' ? info.oldValue as any : null | ||
| updateChangeTrackerForCharacter(targetChar ?? fallbackChar) | ||
| saveTimeoutExecute() | ||
| return | ||
| } | ||
|
|
||
| updateChangeTrackerForCharacter(DBState?.db?.characters?.[selIdState]) | ||
| saveTimeoutExecute() | ||
| }) |
There was a problem hiding this comment.
Multiple rapid database updates could cause a race condition with the changeTracker mutations. The onDatabaseUpdate callback modifies changeTracker arrays using unshift(), and the main saveDb loop reads and modifies these arrays without any locking mechanism. If updates occur while the save loop is processing (between lines 454-458 where changeTracker is being reset), some changes could be lost or duplicated in the tracking arrays.
| const fallbackChar = info.oldValue && typeof info.oldValue === 'object' ? info.oldValue as any : null | ||
| updateChangeTrackerForCharacter(targetChar ?? fallbackChar) |
There was a problem hiding this comment.
The fallback logic for handling character updates when a character is deleted has a type safety issue. The code casts info.oldValue to 'any' without properly validating that it's a character object with the expected structure. If oldValue is not a character object, accessing properties like chaId, chatPage, or chats could fail silently or cause unexpected behavior. Add proper type checking before using the fallback.
| const index = typeof rawIndex === 'number' | ||
| ? rawIndex | ||
| : (typeof rawIndex === 'string' && /^\d+$/.test(rawIndex) ? Number(rawIndex) : selIdState) | ||
| const targetChar = DBState?.db?.characters?.[index ?? selIdState] |
There was a problem hiding this comment.
The character index parsing logic has a potential bug. When rawIndex is a string that doesn't match the digit pattern, it falls back to selIdState. However, this fallback doesn't make sense for all cases - if a non-numeric string property is accessed on the characters array (e.g., 'length' or array methods), using selIdState would incorrectly treat it as a character update. Consider checking if the index is undefined or null before falling back to selIdState, or handling non-numeric properties differently.
| const index = typeof rawIndex === 'number' | |
| ? rawIndex | |
| : (typeof rawIndex === 'string' && /^\d+$/.test(rawIndex) ? Number(rawIndex) : selIdState) | |
| const targetChar = DBState?.db?.characters?.[index ?? selIdState] | |
| let index: number | undefined | |
| if (typeof rawIndex === 'number') { | |
| index = rawIndex | |
| } else if (typeof rawIndex === 'string' && /^\d+$/.test(rawIndex)) { | |
| index = Number(rawIndex) | |
| } | |
| const targetChar = | |
| index !== undefined | |
| ? DBState?.db?.characters?.[index] | |
| : (rawIndex == null ? DBState?.db?.characters?.[selIdState] : undefined) |
| setDatabaseLite(data) | ||
| } | ||
|
|
||
| export function setDatabaseLite(data:Database){ |
There was a problem hiding this comment.
When setDatabaseLite is called with new database data, the old proxies and their event listeners remain in memory because databaseUpdateListeners is a module-level Set that's never cleared. If the database is reloaded multiple times (e.g., during import/restore operations), old listeners will continue to fire for updates to the new database, potentially causing memory leaks and incorrect behavior. Consider clearing databaseUpdateListeners or providing a mechanism to dispose of the old proxy system when a new database is loaded.
| export function setDatabaseLite(data:Database){ | |
| export function setDatabaseLite(data:Database){ | |
| // Clear existing database update listeners so old proxies and their handlers | |
| // do not remain in memory or continue to receive updates after a reload. | |
| if (typeof databaseUpdateListeners !== 'undefined' && databaseUpdateListeners instanceof Set) { | |
| databaseUpdateListeners.clear(); | |
| } |
| return () => { | ||
| unsubscribeSel() | ||
| unsubscribeDb() | ||
| } | ||
| }) |
There was a problem hiding this comment.
The cleanup function returned by the effect.root callback is never actually called or stored anywhere. The saveDb function runs in an infinite loop and never exits, so the cleanup function that unsubscribes from selectedCharID and onDatabaseUpdate is never invoked. This means the listeners will remain active indefinitely, which could cause issues if saveDb is called multiple times or if there's any future refactoring. Consider storing the cleanup function or restructuring the code to ensure proper cleanup.
| import { get } from "svelte/store"; | ||
| import { open } from '@tauri-apps/plugin-shell' | ||
| import { setDatabase, type Database, defaultSdDataFunc, getDatabase, appVer, getCurrentCharacter } from "./storage/database.svelte"; | ||
| import { setDatabase, type Database, defaultSdDataFunc, getDatabase, appVer, getCurrentCharacter, onDatabaseUpdate } from "./storage/database.svelte"; |
There was a problem hiding this comment.
Unused import defaultSdDataFunc.
| import { setDatabase, type Database, defaultSdDataFunc, getDatabase, appVer, getCurrentCharacter, onDatabaseUpdate } from "./storage/database.svelte"; | |
| import { setDatabase, type Database, getDatabase, appVer, getCurrentCharacter, onDatabaseUpdate } from "./storage/database.svelte"; |
PR Checklist
Summary
Improve database save performance by introducing a proxy-based change detection layer, replacing the snapshot-based approach in
saveDb()with event-driven change tracking.Related Issues
None
Changes
onDatabaseUpdate()API indatabase.svelte.tsfor reactive database mutation detection.The callback receives
DatabaseUpdateInfocontainingpath,value,oldValueandtypesaveDb()to managechangeTrackerusing event-based callbacks fromonDatabaseUpdate()instead of$state.snapshotoperations.Impact
O(n)snapshot scanning toO(1)reactive event handlingAdditional Notes
Tested on various edge cases, including character/chat/message/prompt/module addition and deletion. Further testing may be beneficial.
Footnotes
Modifies the behavior of prompting, requesting or handling responses from ai models. ↩
Almost over 80% of the code is ai generated. ↩