From 618e029ff49fbbfd1dc45f515779d74d53af30a9 Mon Sep 17 00:00:00 2001 From: Jon Kazama Date: Tue, 16 Jun 2026 20:39:56 +0200 Subject: [PATCH] perf(tabs): share Plugin.TabsListLock across AutoTellTabsService + MessageManager refilter + SaveConfig (B3) --- HellionChat/AutoTellTabsService.cs | 76 +++++++++++++++++------------- HellionChat/MessageManager.cs | 50 +++++++++++++++----- HellionChat/Plugin.cs | 31 ++++++++---- 3 files changed, 103 insertions(+), 54 deletions(-) diff --git a/HellionChat/AutoTellTabsService.cs b/HellionChat/AutoTellTabsService.cs index f66eed5..8e3e8e3 100644 --- a/HellionChat/AutoTellTabsService.cs +++ b/HellionChat/AutoTellTabsService.cs @@ -21,7 +21,10 @@ internal sealed class AutoTellTabsService : IDisposable private readonly MessageManager _messageManager; private readonly MessageStore _store; private readonly ILogger _logger; - private readonly object _tempTabsLock = new(); + + // Tabs-list structure lock now lives on Plugin (neutral owner) so the + // MessageManager refilter can share it. See Plugin.TabsListLock / B3. + private object TabsListLock => _plugin.TabsListLock; // Hard cap on pinned TempTabs so the sidebar doesn't inflate over years // of usage. Separate pool from AutoTellTabsLimit (15) — pinned tabs live @@ -147,7 +150,7 @@ internal sealed class AutoTellTabsService : IDisposable return; } - lock (_tempTabsLock) + lock (TabsListLock) { var existing = FindTempTab(partner.Value.Name, partner.Value.World); if (existing != null) @@ -240,46 +243,51 @@ internal sealed class AutoTellTabsService : IDisposable } // Lock-protected lookup for the framework-thread caller (TellRouterService). - // Config.Tabs is mutated under _tempTabsLock on the PendingMessage worker thread, + // Config.Tabs is mutated under the shared Plugin.TabsListLock on the worker thread, // so a framework-tick reader must take the same lock to avoid enumerating the list // mid-mutation. internal Tab? FindTempTabSafe(string name, uint world) { - lock (_tempTabsLock) + lock (TabsListLock) return FindTempTab(name, world); } internal void DropOldestTempTab() { - // Pinned tabs live in their own bucket (MaxPinnedTempTabs) and are - // never drop candidates. They leave the bucket only via Unpin or - // PromoteToPermanent. - var victim = Plugin - .Config.Tabs.Select((tab, idx) => (Tab: tab, Index: idx)) - .Where(t => TabLifecycleHelpers.IsInUnpinnedPool(t.Tab)) - .OrderByDescending(t => t.Tab.IsGreeted) - .ThenBy(t => t.Tab.LastActivity) - .FirstOrDefault(); - - if (victim.Tab == null) + // B3: lock the list-structure ops so the (currently caller-less) Unpin path + // can't race the worker; re-entrant when HandleTell already holds the lock. + lock (TabsListLock) { - return; + // Pinned tabs live in their own bucket (MaxPinnedTempTabs) and are + // never drop candidates. They leave the bucket only via Unpin or + // PromoteToPermanent. + var victim = Plugin + .Config.Tabs.Select((tab, idx) => (Tab: tab, Index: idx)) + .Where(t => TabLifecycleHelpers.IsInUnpinnedPool(t.Tab)) + .OrderByDescending(t => t.Tab.IsGreeted) + .ThenBy(t => t.Tab.LastActivity) + .FirstOrDefault(); + + if (victim.Tab == null) + { + return; + } + + var dropped = victim.Tab; + Plugin.Config.Tabs.RemoveAt(victim.Index); + + // Re-anchor the UI selection if it pointed at the dropped tab, and close any + // pop-out window the dropped tab owned. Both run on the PendingMessage worker + // thread and touch window state the Draw path reads (OnTabActivated re-seed + + // the pool's Unbind), so marshal onto the framework thread to serialize with + // Draw (reference_dalamud_framework_thread). TryClose is idempotent: a tab that + // was never popped is a silent no-op. + Plugin.Framework.RunOnFrameworkThread(() => + { + _plugin.ChannelPopoutPool.TryClose(dropped.Identifier); + _plugin.MainWindow?.ResetActiveTabIfRemoved(dropped); + }); } - - var dropped = victim.Tab; - Plugin.Config.Tabs.RemoveAt(victim.Index); - - // Re-anchor the UI selection if it pointed at the dropped tab, and close any - // pop-out window the dropped tab owned. Both run on the PendingMessage worker - // thread and touch window state the Draw path reads (OnTabActivated re-seed + - // the pool's Unbind), so marshal onto the framework thread to serialize with - // Draw (reference_dalamud_framework_thread). TryClose is idempotent: a tab that - // was never popped is a silent no-op. - Plugin.Framework.RunOnFrameworkThread(() => - { - _plugin.ChannelPopoutPool.TryClose(dropped.Identifier); - _plugin.MainWindow?.ResetActiveTabIfRemoved(dropped); - }); } private void SpawnTempTab((string Name, uint World) partner, Message currentMessage) @@ -302,7 +310,7 @@ internal sealed class AutoTellTabsService : IDisposable // Actually open the pop-out window for the flagged tab — without this the // flag was dead (a PopOut tab with no window). SpawnTempTab runs on the - // PendingMessage worker thread under _tempTabsLock; TryOpen does + // PendingMessage worker thread under Plugin.TabsListLock; TryOpen does // OnTabActivated + Bind (window state Draw reads), so marshal onto the // framework thread. If the pool is full, drop the flag so it never claims a // window it didn't get (flag/window parity). @@ -428,7 +436,7 @@ internal sealed class AutoTellTabsService : IDisposable return; } - lock (_tempTabsLock) + lock (TabsListLock) { // Guard against frame-race: sidebar might render a tab already removed by LRU or logout if (!Plugin.Config.Tabs.Contains(tab)) @@ -442,7 +450,7 @@ internal sealed class AutoTellTabsService : IDisposable private void OnLogout(int type, int code) { - lock (_tempTabsLock) + lock (TabsListLock) { // Pinned TempTabs must survive char-switch — that's the whole point // of pinning. Only unpinned ones get stripped. diff --git a/HellionChat/MessageManager.cs b/HellionChat/MessageManager.cs index 2acf577..dcb10a2 100644 --- a/HellionChat/MessageManager.cs +++ b/HellionChat/MessageManager.cs @@ -163,8 +163,15 @@ internal class MessageManager : IAsyncDisposable internal void ClearAllTabs() { + // B3: snapshot the tab LIST under the shared lock so the worker-thread + // add/remove can't tear the enumeration; tab.Clear() then runs lock-free + // (each tab's Messages has its own SemaphoreSlim — lock order: list outer). + List tabsSnapshot; + lock (Plugin.TabsListLock) + tabsSnapshot = Plugin.Config.Tabs.ToList(); + // TempTabs are session-only (not persisted); exclude them to preserve Tell history - foreach (var tab in Plugin.Config.Tabs.Where(t => !t.IsTempTab)) + foreach (var tab in tabsSnapshot.Where(t => !t.IsTempTab)) tab.Clear(); } @@ -176,18 +183,19 @@ internal class MessageManager : IAsyncDisposable using var messages = Store.GetMostRecentMessages(CurrentContentId, since); - // TempTabs are excluded; they maintain live state from AutoTellTabsService - var pendingTabs = Plugin - .Config.Tabs.Where(t => !t.IsTempTab) - .Select(tab => (tab, new List())) - .ToList(); - foreach (var message in messages) - foreach (var (_, pendingMessages) in pendingTabs.Where(ptab => ptab.Item1.Matches(message))) - pendingMessages.Add(message); + // TempTabs excluded (live state from AutoTellTabsService). Bucket via the + // pure MapMessagesToTabs so the assignment stays testable outside Dalamud (B3-1). + // B3: snapshot under the shared lock (list copy only — short critical + // section). The Store query above and the AddSortPrune writes below stay + // OUTSIDE the lock (lock order: list outer, MessageList inner). + List nonTempTabs; + lock (Plugin.TabsListLock) + nonTempTabs = Plugin.Config.Tabs.Where(t => !t.IsTempTab).ToList(); + var buckets = MapMessagesToTabs(nonTempTabs, messages); // Apply messages to chat log all at once. - foreach (var (tab, pendingMessages) in pendingTabs) - tab.Messages.AddSortPrune(pendingMessages, MessageDisplayLimit); + foreach (var tab in nonTempTabs) + tab.Messages.AddSortPrune(buckets[tab], MessageDisplayLimit); if (!messages.DidError) return; @@ -206,6 +214,26 @@ internal class MessageManager : IAsyncDisposable } } + // Pure message->tab bucketing for the refilter. Dalamud-free + static so the + // assignment can be unit-pinned in the build suite; the live caller owns the + // Store query, the snapshot and the SemaphoreSlim writes. + internal static Dictionary> MapMessagesToTabs( + IReadOnlyList tabs, + IEnumerable messages + ) + { + var buckets = new Dictionary>(tabs.Count); + foreach (var tab in tabs) + buckets[tab] = new List(); + + foreach (var message in messages) + foreach (var tab in tabs) + if (tab.Matches(message)) + buckets[tab].Add(message); + + return buckets; + } + internal void FilterAllTabsAsync() { Task.Run(() => diff --git a/HellionChat/Plugin.cs b/HellionChat/Plugin.cs index 22deb6e..5f3dc27 100755 --- a/HellionChat/Plugin.cs +++ b/HellionChat/Plugin.cs @@ -189,6 +189,12 @@ public sealed class Plugin : IAsyncDalamudPlugin internal readonly object RetentionSweepLock = new(); internal volatile bool RetentionSweepRunning; + // B3: neutral owner of the Config.Tabs LIST-structure lock so both the + // worker-thread mutator (AutoTellTabsService) and the framework-thread + // refilter (MessageManager) share ONE monitor. Lock order: this outer, + // MessageList's SemaphoreSlim inner — never the reverse. + internal readonly object TabsListLock = new(); + internal DateTime GameStarted { get; } // Couples "current tab" to the real UI selection. The chat hooks are @@ -976,11 +982,10 @@ public sealed class Plugin : IAsyncDalamudPlugin { Log.Information($"Retention sweep deleted {deleted} expired messages."); // Schedule on the next framework tick to avoid the ~194ms - // hitch from blocking with .Wait() while the framework - // finishes the current frame. Tabs-list mutation must - // stay on the framework thread because Plugin.Config.Tabs - // (Configuration.cs:222) is not lock-protected and - // AutoTellTabsService can mutate it from background paths. + // hitch from blocking with .Wait() while the frame finishes. + // The Config.Tabs enumeration in ClearAllTabs/FilterAllTabs is + // now guarded by the shared Plugin.TabsListLock (B3), so this + // tick scheduling is purely hitch-avoidance, not safety. // Pattern reference: SimpleTweaks // Tweaks/Chat/CaseInsensitiveCommands.cs:45. Framework.RunOnTick(() => @@ -1091,12 +1096,20 @@ public sealed class Plugin : IAsyncDalamudPlugin // Config.Tabs across the save so JSON includes them. Cloning only the // unpinned subset keeps the allocation proportional to // AutoTellTabsLimit (<=15) instead of the full tab list. - var unpinnedTempTabs = Config.Tabs.Where(TabLifecycleHelpers.IsInUnpinnedPool).ToList(); - Config.Tabs.RemoveAll(TabLifecycleHelpers.ShouldStripOnSave); + // B3: the strip/restore mutates the tab LIST, so it shares TabsListLock + // with the worker add/remove and the refilter snapshot. Re-entrant: the + // one worker caller (HandleTell) already holds it; framework callers take + // it here. SavePluginConfig runs inside (short, in-memory) — the §8 fallback + // (serialize a copy outside the lock) is a tracked pre-beta to-do. + lock (TabsListLock) + { + var unpinnedTempTabs = Config.Tabs.Where(TabLifecycleHelpers.IsInUnpinnedPool).ToList(); + Config.Tabs.RemoveAll(TabLifecycleHelpers.ShouldStripOnSave); - Interface.SavePluginConfig(Config); + Interface.SavePluginConfig(Config); - Config.Tabs.AddRange(unpinnedTempTabs); + Config.Tabs.AddRange(unpinnedTempTabs); + } } internal void LanguageChanged(string langCode)