perf(favicon): eliminate redundant processing on refresh
- Check WebKit's FaviconDatabase before processing to skip cached URIs - Add deduplication window to prevent PNG/SVG/ICO flip-flopping - Make domain propagation async to avoid blocking page loads - Remove duplicate favicon log in title bar This fixes page load delays caused by synchronous database operations (2000+ queries) being triggered on every refresh.
- commit
- adbefcc9b6bd89ede1bfabf49824156c97bb0980
- author
- bnema · · 2025-11-13 06:48 UTC
- parent
- c4a3fff
- signature
- unsigned
3 files · +60 -11
M CHANGELOG.md +1 -0
| @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file. | |||
| 8 | 8 | - **Middle-click and Ctrl+click link handling**: Links can now be opened in new workspace panes using standard browser gestures (middle-click or Ctrl+left-click). New panes split in the configured direction (default: right) and respect `workspace.popups.placement` configuration. Implemented via WebKit's `decide-policy` signal to intercept navigation actions before they occur. | |
| 9 | 9 | ||
| 10 | 10 | ### Fixed | |
| 11 | + | - **Favicon processing on refresh**: Skip redundant favicon processing when WebKit's cache already has the URI. Made domain propagation async to prevent blocking page loads. | |
| 11 | 12 | - **Popup handler registration timing**: Fixed `g_object_ref` assertion errors when WebKit takes ownership of popup WebViews by connecting signals immediately instead of deferring via `glib.IdleAdd`. This resolves crashes with `target="_blank"` links and `window.open()` calls. | |
| 12 | 13 | ||
| 13 | 14 | ## [0.13.0] - 2025-10-24 | |
| @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file. | |||||
| 8 | - **Middle-click and Ctrl+click link handling**: Links can now be opened in new workspace panes using standard browser gestures (middle-click or Ctrl+left-click). New panes split in the configured direction (default: right) and respect `workspace.popups.placement` configuration. Implemented via WebKit's `decide-policy` signal to intercept navigation actions before they occur. | 8 | - **Middle-click and Ctrl+click link handling**: Links can now be opened in new workspace panes using standard browser gestures (middle-click or Ctrl+left-click). New panes split in the configured direction (default: right) and respect `workspace.popups.placement` configuration. Implemented via WebKit's `decide-policy` signal to intercept navigation actions before they occur. | ||
| 9 | 9 | ||||
| 10 | ### Fixed | 10 | ### Fixed | ||
| 11 | + | - **Favicon processing on refresh**: Skip redundant favicon processing when WebKit's cache already has the URI. Made domain propagation async to prevent blocking page loads. | |||
| 11 | - **Popup handler registration timing**: Fixed `g_object_ref` assertion errors when WebKit takes ownership of popup WebViews by connecting signals immediately instead of deferring via `glib.IdleAdd`. This resolves crashes with `target="_blank"` links and `window.open()` calls. | 12 | - **Popup handler registration timing**: Fixed `g_object_ref` assertion errors when WebKit takes ownership of popup WebViews by connecting signals immediately instead of deferring via `glib.IdleAdd`. This resolves crashes with `target="_blank"` links and `window.open()` calls. | ||
| 12 | 13 | ||||
| 13 | ## [0.13.0] - 2025-10-24 | 14 | ## [0.13.0] - 2025-10-24 | ||
M internal/app/browser/stacked_panes.go +0 -1
| @@ -678,7 +678,6 @@ func (spm *StackedPaneManager) createTitleBarWithTitle(title string, pageURL str | |||
| 678 | 678 | } | |
| 679 | 679 | ||
| 680 | 680 | if texture != nil && titleBox != nil { | |
| 681 | - | log.Printf("[favicon] Favicon ready to display in title bar for: %s", pageURL) | |
| 682 | 681 | ||
| 683 | 682 | // Create favicon image | |
| 684 | 683 | faviconImg := gtk.NewImageFromPaintable(texture) | |
| @@ -678,7 +678,6 @@ func (spm *StackedPaneManager) createTitleBarWithTitle(title string, pageURL str | |||||
| 678 | } | 678 | } | ||
| 679 | 679 | ||||
| 680 | if texture != nil && titleBox != nil { | 680 | if texture != nil && titleBox != nil { | ||
| 681 | - | log.Printf("[favicon] Favicon ready to display in title bar for: %s", pageURL) | |||
| 682 | 681 | ||||
| 683 | // Create favicon image | 682 | // Create favicon image | ||
| 684 | faviconImg := gtk.NewImageFromPaintable(texture) | 683 | faviconImg := gtk.NewImageFromPaintable(texture) | ||
M internal/services/favicon_service.go +59 -10
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | "log" | |
| 10 | 10 | "os" | |
| 11 | 11 | "path/filepath" | |
| 12 | + | "sync" | |
| 12 | 13 | "time" | |
| 13 | 14 | ||
| 14 | 15 | "github.com/bnema/dumber/internal/db" | |
| @@ -28,6 +29,14 @@ type FaviconService struct { | |||
| 28 | 29 | exportDir string // Directory for exporting favicons for CLI access | |
| 29 | 30 | dataDir string // WebKit data directory | |
| 30 | 31 | enableExport bool // Whether to export favicons for CLI tools | |
| 32 | + | ||
| 33 | + | // Deduplication: tracks recently processed (pageURL, faviconURI) pairs | |
| 34 | + | // Key: pageURL, Value: struct{faviconURI string, timestamp time.Time} | |
| 35 | + | recentChanges sync.Map | |
| 36 | + | ||
| 37 | + | // Debouncing: tracks ongoing preload operations to prevent duplicates | |
| 38 | + | // Key: pageURL, Value: bool (true if preload is in progress) | |
| 39 | + | ongoingPreloads sync.Map | |
| 31 | 40 | } | |
| 32 | 41 | ||
| 33 | 42 | const ( | |
| @@ -35,6 +44,8 @@ const ( | |||
| 35 | 44 | DefaultFaviconSize = 32 | |
| 36 | 45 | // FaviconTimeout is the maximum time to wait for a favicon to be available | |
| 37 | 46 | FaviconTimeout = 5 * time.Second | |
| 47 | + | // DeduplicationWindow is the time window for ignoring duplicate favicon changes | |
| 48 | + | DeduplicationWindow = 2 * time.Second | |
| 38 | 49 | ) | |
| 39 | 50 | ||
| 40 | 51 | // ServiceName returns the service name for identification | |
| @@ -83,28 +94,66 @@ func NewFaviconService(faviconDB *webkit.FaviconDatabase, queries db.DatabaseQue | |||
| 83 | 94 | // OnFaviconChanged handles favicon URI changes from WebKit's favicon database. | |
| 84 | 95 | // We store the URI (PNG, SVG, etc.) so that callers can later load an appropriate texture. | |
| 85 | 96 | func (fs *FaviconService) OnFaviconChanged(pageURL, faviconURI string) error { | |
| 86 | - | log.Printf("[favicon] Favicon changed for %s: %s", pageURL, faviconURI) | |
| 87 | - | ||
| 88 | - | ctx := context.Background() | |
| 89 | - | ||
| 90 | 97 | if !fs.shouldProcessURL(pageURL) { | |
| 91 | 98 | return nil | |
| 92 | 99 | } | |
| 93 | 100 | ||
| 101 | + | // FIRST: Check if WebKit's FaviconDatabase already has this exact URI | |
| 102 | + | // WebKit's database persists across sessions and handles caching internally | |
| 103 | + | // If it's already stored, this is just a notification on refresh/reload - skip it | |
| 104 | + | existingURI := fs.faviconDB.FaviconURI(pageURL) | |
| 105 | + | if existingURI == faviconURI { | |
| 106 | + | // WebKit already has this favicon cached - nothing to do | |
| 107 | + | // This is the common case on refresh/new window | |
| 108 | + | return nil | |
| 109 | + | } | |
| 110 | + | ||
| 111 | + | // Deduplication for rapid-fire events on initial page load | |
| 112 | + | // (e.g., GitHub fires PNG then SVG within milliseconds) | |
| 113 | + | // Keep the first one we see and ignore subsequent variants | |
| 114 | + | type recentChange struct { | |
| 115 | + | faviconURI string | |
| 116 | + | timestamp time.Time | |
| 117 | + | } | |
| 118 | + | ||
| 119 | + | if cached, ok := fs.recentChanges.Load(pageURL); ok { | |
| 120 | + | if rc, ok := cached.(recentChange); ok { | |
| 121 | + | if time.Since(rc.timestamp) < DeduplicationWindow { | |
| 122 | + | // Already processed a favicon for this URL recently | |
| 123 | + | // This prevents flip-flopping between PNG/SVG/ICO variants | |
| 124 | + | return nil | |
| 125 | + | } | |
| 126 | + | } | |
| 127 | + | } | |
| 128 | + | ||
| 129 | + | // NEW favicon detected (first time or actual change) | |
| 130 | + | log.Printf("[favicon] Favicon changed for %s: %s", pageURL, faviconURI) | |
| 131 | + | ||
| 132 | + | ctx := context.Background() | |
| 133 | + | ||
| 134 | + | // Update our history database with the favicon URI | |
| 94 | 135 | nullString := sql.NullString{String: faviconURI, Valid: faviconURI != ""} | |
| 95 | 136 | if err := fs.dbQueries.UpdateHistoryFavicon(ctx, nullString, pageURL); err != nil { | |
| 96 | 137 | log.Printf("[favicon] Failed to update favicon URI in database for %s: %v", pageURL, err) | |
| 97 | 138 | return fmt.Errorf("failed to update favicon in database: %w", err) | |
| 98 | 139 | } | |
| 99 | 140 | ||
| 100 | - | // Also update all other URLs from the same domain to propagate favicon | |
| 101 | - | // This handles multiple paths on the same domain (e.g., google.com/search?q=foo and google.com/search?q=bar) | |
| 102 | - | fs.propagateFaviconToDomain(pageURL, faviconURI) | |
| 141 | + | // Store this change as recently processed | |
| 142 | + | fs.recentChanges.Store(pageURL, recentChange{faviconURI: faviconURI, timestamp: time.Now()}) | |
| 103 | 143 | ||
| 104 | - | // Proactively render and export favicon for omnibox/CLI use | |
| 105 | - | // This ensures favicons are available ASAP for suggestions | |
| 144 | + | // Propagate favicon to domain asynchronously to avoid blocking WebKit | |
| 145 | + | go fs.propagateFaviconToDomain(pageURL, faviconURI) | |
| 146 | + | ||
| 147 | + | // Proactively render and export favicon for omnibox/CLI use (debounced) | |
| 106 | 148 | if fs.enableExport { | |
| 107 | - | go fs.preloadFavicon(pageURL, faviconURI) | |
| 149 | + | // Check if a preload is already in progress for this URL | |
| 150 | + | if _, loaded := fs.ongoingPreloads.LoadOrStore(pageURL, true); !loaded { | |
| 151 | + | // No preload in progress, start one | |
| 152 | + | go func() { | |
| 153 | + | defer fs.ongoingPreloads.Delete(pageURL) | |
| 154 | + | fs.preloadFavicon(pageURL, faviconURI) | |
| 155 | + | }() | |
| 156 | + | } | |
| 108 | 157 | } | |
| 109 | 158 | ||
| 110 | 159 | return nil | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | "log" | 9 | "log" | ||
| 10 | "os" | 10 | "os" | ||
| 11 | "path/filepath" | 11 | "path/filepath" | ||
| 12 | + | "sync" | |||
| 12 | "time" | 13 | "time" | ||
| 13 | 14 | ||||
| 14 | "github.com/bnema/dumber/internal/db" | 15 | "github.com/bnema/dumber/internal/db" | ||
| @@ -28,6 +29,14 @@ type FaviconService struct { | |||||
| 28 | exportDir string // Directory for exporting favicons for CLI access | 29 | exportDir string // Directory for exporting favicons for CLI access | ||
| 29 | dataDir string // WebKit data directory | 30 | dataDir string // WebKit data directory | ||
| 30 | enableExport bool // Whether to export favicons for CLI tools | 31 | enableExport bool // Whether to export favicons for CLI tools | ||
| 32 | + | ||||
| 33 | + | // Deduplication: tracks recently processed (pageURL, faviconURI) pairs | |||
| 34 | + | // Key: pageURL, Value: struct{faviconURI string, timestamp time.Time} | |||
| 35 | + | recentChanges sync.Map | |||
| 36 | + | ||||
| 37 | + | // Debouncing: tracks ongoing preload operations to prevent duplicates | |||
| 38 | + | // Key: pageURL, Value: bool (true if preload is in progress) | |||
| 39 | + | ongoingPreloads sync.Map | |||
| 31 | } | 40 | } | ||
| 32 | 41 | ||||
| 33 | const ( | 42 | const ( | ||
| @@ -35,6 +44,8 @@ const ( | |||||
| 35 | DefaultFaviconSize = 32 | 44 | DefaultFaviconSize = 32 | ||
| 36 | // FaviconTimeout is the maximum time to wait for a favicon to be available | 45 | // FaviconTimeout is the maximum time to wait for a favicon to be available | ||
| 37 | FaviconTimeout = 5 * time.Second | 46 | FaviconTimeout = 5 * time.Second | ||
| 47 | + | // DeduplicationWindow is the time window for ignoring duplicate favicon changes | |||
| 48 | + | DeduplicationWindow = 2 * time.Second | |||
| 38 | ) | 49 | ) | ||
| 39 | 50 | ||||
| 40 | // ServiceName returns the service name for identification | 51 | // ServiceName returns the service name for identification | ||
| @@ -83,28 +94,66 @@ func NewFaviconService(faviconDB *webkit.FaviconDatabase, queries db.DatabaseQue | |||||
| 83 | // OnFaviconChanged handles favicon URI changes from WebKit's favicon database. | 94 | // OnFaviconChanged handles favicon URI changes from WebKit's favicon database. | ||
| 84 | // We store the URI (PNG, SVG, etc.) so that callers can later load an appropriate texture. | 95 | // We store the URI (PNG, SVG, etc.) so that callers can later load an appropriate texture. | ||
| 85 | func (fs *FaviconService) OnFaviconChanged(pageURL, faviconURI string) error { | 96 | func (fs *FaviconService) OnFaviconChanged(pageURL, faviconURI string) error { | ||
| 86 | - | log.Printf("[favicon] Favicon changed for %s: %s", pageURL, faviconURI) | |||
| 87 | - | ||||
| 88 | - | ctx := context.Background() | |||
| 89 | - | ||||
| 90 | if !fs.shouldProcessURL(pageURL) { | 97 | if !fs.shouldProcessURL(pageURL) { | ||
| 91 | return nil | 98 | return nil | ||
| 92 | } | 99 | } | ||
| 93 | 100 | ||||
| 101 | + | // FIRST: Check if WebKit's FaviconDatabase already has this exact URI | |||
| 102 | + | // WebKit's database persists across sessions and handles caching internally | |||
| 103 | + | // If it's already stored, this is just a notification on refresh/reload - skip it | |||
| 104 | + | existingURI := fs.faviconDB.FaviconURI(pageURL) | |||
| 105 | + | if existingURI == faviconURI { | |||
| 106 | + | // WebKit already has this favicon cached - nothing to do | |||
| 107 | + | // This is the common case on refresh/new window | |||
| 108 | + | return nil | |||
| 109 | + | } | |||
| 110 | + | ||||
| 111 | + | // Deduplication for rapid-fire events on initial page load | |||
| 112 | + | // (e.g., GitHub fires PNG then SVG within milliseconds) | |||
| 113 | + | // Keep the first one we see and ignore subsequent variants | |||
| 114 | + | type recentChange struct { | |||
| 115 | + | faviconURI string | |||
| 116 | + | timestamp time.Time | |||
| 117 | + | } | |||
| 118 | + | ||||
| 119 | + | if cached, ok := fs.recentChanges.Load(pageURL); ok { | |||
| 120 | + | if rc, ok := cached.(recentChange); ok { | |||
| 121 | + | if time.Since(rc.timestamp) < DeduplicationWindow { | |||
| 122 | + | // Already processed a favicon for this URL recently | |||
| 123 | + | // This prevents flip-flopping between PNG/SVG/ICO variants | |||
| 124 | + | return nil | |||
| 125 | + | } | |||
| 126 | + | } | |||
| 127 | + | } | |||
| 128 | + | ||||
| 129 | + | // NEW favicon detected (first time or actual change) | |||
| 130 | + | log.Printf("[favicon] Favicon changed for %s: %s", pageURL, faviconURI) | |||
| 131 | + | ||||
| 132 | + | ctx := context.Background() | |||
| 133 | + | ||||
| 134 | + | // Update our history database with the favicon URI | |||
| 94 | nullString := sql.NullString{String: faviconURI, Valid: faviconURI != ""} | 135 | nullString := sql.NullString{String: faviconURI, Valid: faviconURI != ""} | ||
| 95 | if err := fs.dbQueries.UpdateHistoryFavicon(ctx, nullString, pageURL); err != nil { | 136 | if err := fs.dbQueries.UpdateHistoryFavicon(ctx, nullString, pageURL); err != nil { | ||
| 96 | log.Printf("[favicon] Failed to update favicon URI in database for %s: %v", pageURL, err) | 137 | log.Printf("[favicon] Failed to update favicon URI in database for %s: %v", pageURL, err) | ||
| 97 | return fmt.Errorf("failed to update favicon in database: %w", err) | 138 | return fmt.Errorf("failed to update favicon in database: %w", err) | ||
| 98 | } | 139 | } | ||
| 99 | 140 | ||||
| 100 | - | // Also update all other URLs from the same domain to propagate favicon | 141 | + | // Store this change as recently processed |
| 101 | - | // This handles multiple paths on the same domain (e.g., google.com/search?q=foo and google.com/search?q=bar) | 142 | + | fs.recentChanges.Store(pageURL, recentChange{faviconURI: faviconURI, timestamp: time.Now()}) |
| 102 | - | fs.propagateFaviconToDomain(pageURL, faviconURI) | |||
| 103 | 143 | ||||
| 104 | - | // Proactively render and export favicon for omnibox/CLI use | 144 | + | // Propagate favicon to domain asynchronously to avoid blocking WebKit |
| 105 | - | // This ensures favicons are available ASAP for suggestions | 145 | + | go fs.propagateFaviconToDomain(pageURL, faviconURI) |
| 146 | + | ||||
| 147 | + | // Proactively render and export favicon for omnibox/CLI use (debounced) | |||
| 106 | if fs.enableExport { | 148 | if fs.enableExport { | ||
| 107 | - | go fs.preloadFavicon(pageURL, faviconURI) | 149 | + | // Check if a preload is already in progress for this URL |
| 150 | + | if _, loaded := fs.ongoingPreloads.LoadOrStore(pageURL, true); !loaded { | |||
| 151 | + | // No preload in progress, start one | |||
| 152 | + | go func() { | |||
| 153 | + | defer fs.ongoingPreloads.Delete(pageURL) | |||
| 154 | + | fs.preloadFavicon(pageURL, faviconURI) | |||
| 155 | + | }() | |||
| 156 | + | } | |||
| 108 | } | 157 | } | ||
| 109 | 158 | ||||
| 110 | return nil | 159 | return nil | ||