JonKazama-Hellion/HellionChat
1
0
Fork 0
Code Issues 1 Pull Requests 3 Actions Releases 49 Activity

perf(tells): build the tab outside the lock, guard pin transitions

Browse Source 
HandleTell was one atomic block, and PreloadHistory sat inside it -- so every
new tell partner held TabsListLock across a store query that sorted the whole
receiver history before returning a row. That is the lock the draw thread and
the message worker both wait on.

Now three steps: look for an existing tab under the lock, build the new one
(including history) without it, then commit under the lock again. Splitting it
opens a window where the world can change, so the second block re-checks:

- FindTempTab again, in case something else created the tab meanwhile. The
  message goes to that one instead. Not in the first block's early return --
  HandleTell runs after the delivery loop, so an existing tab already has it and
  adding again would duplicate the line.
- A generation counter, bumped by OnLogout under the same lock. A logout in
  between wipes the unpinned pool, and without this the freshly built tab would
  outlive it and show up for a character we already left. Not via
  CurrentContentId: its getter falls back to a cached value, so the comparison
  can silently pass.
- The pool cap moves into CommitTempTab and stays there exactly once. Evaluating
  it twice would evict a tab on every spawn.

Pin, unpin and promote take the lock around the flag change now -- they decide
pool membership and whether a save strips the tab. SaveConfig stays outside, so
no fsync lands on the click path.

DropOldestTempTab removes by reference: the index came from an earlier Select in
the same block and would point at the wrong tab if anything shifted the list.
This commit is contained in:
JonKazama-Hellion
2026-08-17 18:38:08 +02:00
parent c34024a18b
commit 3583dfc032
1 changed files with 86 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
HellionChat/AutoTellTabsService.cs
+86 -22
View File
@@ -26,6 +26,11 @@ internal sealed class AutoTellTabsService : IDisposable
// MessageManager refilter can share it. See Plugin.TabsListLock / B3.
private object TabsListLock => _plugin.TabsListLock;
// Bumped whenever something wipes unpinned temp tabs wholesale (logout).
// HandleTell reads it before releasing the lock and re-checks after, so a
// tab built in between is discarded instead of outliving the wipe.
private int _tabGeneration;
// Hard cap on pinned TempTabs so the sidebar doesn't inflate over years
// of usage. Separate pool from AutoTellTabsLimit (15) — pinned tabs live
// in their own bucket. A configurable cap is a vault-backlog anchor for
@@ -150,15 +155,19 @@ internal sealed class AutoTellTabsService : IDisposable
return;
}
// Three steps, because building the tab pulls history out of the store and
// that must not happen under TabsListLock (B3 rule; the query sorts the whole
// receiver history). Step 1 and 3 are locked, step 2 is not.
int generation;
lock (TabsListLock)
{
var existing = FindTempTab(partner.Value.Name, partner.Value.World);
if (existing != null)
{
// Already routed via MessageManager pipeline. Repair the
// tell-target if the fallback hit a pinned tab whose
// TellTarget didn't survive a previous round-trip — keeps
// FindTempTab fast on the next message.
// Already routed via MessageManager pipeline — no AddMessage here,
// HandleTell runs after the delivery loop. Repair the tell-target if
// the fallback hit a pinned tab whose TellTarget didn't survive a
// previous round-trip — keeps FindTempTab fast on the next message.
if (
existing.IsPinned
&& (existing.TellTarget is null || !existing.TellTarget.IsSet())
@@ -175,12 +184,29 @@ internal sealed class AutoTellTabsService : IDisposable
return;
}
if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit)
generation = _tabGeneration;
}
var tab = BuildTempTabWithHistory(partner.Value, message);
lock (TabsListLock)
{
// A logout in between wiped the unpinned pool; committing now would
// resurrect a tab for a character we already left.
if (generation != _tabGeneration)
return;
// Someone else (self-test, UI) may have created the tab while we built
// ours. Hand the message to theirs and drop what we built — unlike the
// early return above, this tab appeared after the delivery loop ran.
var raced = FindTempTab(partner.Value.Name, partner.Value.World);
if (raced != null)
{
DropOldestTempTab();
raced.AddMessage(message, unread: true);
return;
}
SpawnTempTab(partner.Value, message);
CommitTempTab(tab);
}
}
@@ -274,7 +300,9 @@ internal sealed class AutoTellTabsService : IDisposable
}
var dropped = victim.Tab;
Plugin.Config.Tabs.RemoveAt(victim.Index);
// By reference, not by index: the index came from a Select() earlier in
// this block and would point at the wrong tab if anything shifted the list.
Plugin.Config.Tabs.Remove(dropped);
// 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
@@ -290,7 +318,10 @@ internal sealed class AutoTellTabsService : IDisposable
}
}
private void SpawnTempTab((string Name, uint World) partner, Message currentMessage)
// Runs WITHOUT TabsListLock: PreloadHistory hits the store, which used to hold
// the lock across a query that sorted the whole receiver history. The tab is not
// public until CommitTempTab adds it, so building it unlocked is safe.
private Tab BuildTempTabWithHistory((string Name, uint World) partner, Message currentMessage)
{
var tab = BuildTempTab(partner.Name, partner.World);
@@ -306,10 +337,21 @@ internal sealed class AutoTellTabsService : IDisposable
tab.PopOut = true;
}
return tab;
}
// Caller MUST hold TabsListLock.
private void CommitTempTab(Tab tab)
{
if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit)
{
DropOldestTempTab();
}
Plugin.Config.Tabs.Add(tab);
// 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
// flag was dead (a PopOut tab with no window). CommitTempTab runs on the
// 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
@@ -472,6 +514,11 @@ internal sealed class AutoTellTabsService : IDisposable
Plugin.Config.Tabs.RemoveAll(TabLifecycleHelpers.IsInUnpinnedPool);
// HandleTell builds a tab outside the lock; bumping here lets it detect
// that the world moved on and drop what it built. Read and compared under
// the same lock, so no volatile needed.
_tabGeneration++;
// Re-anchor the UI selection if the active tab was one of the stripped
// unpinned temp tabs (reference predicate, not an index). Logout is a
// framework-thread event, so this is already serialized with Draw — no
@@ -493,16 +540,23 @@ internal sealed class AutoTellTabsService : IDisposable
return false;
}
if (PinnedTempTabCount >= MaxPinnedTempTabs)
// Count and flag under one lock so the cap can't be raced. SaveConfig stays
// OUTSIDE -- holding TabsListLock across a save would put an fsync on the
// click path, which is what B6 just removed elsewhere.
lock (TabsListLock)
{
WrapperUtil.AddNotification(
string.Format(HellionStrings.PinTab_LimitReached, MaxPinnedTempTabs),
NotificationType.Warning
);
return false;
if (PinnedTempTabCount >= MaxPinnedTempTabs)
{
WrapperUtil.AddNotification(
string.Format(HellionStrings.PinTab_LimitReached, MaxPinnedTempTabs),
NotificationType.Warning
);
return false;
}
tab.IsPinned = true;
}
tab.IsPinned = true;
_logger.LogDebug(
$"[Pin] Pinned tab '{tab.Name}' target={tab.TellTarget?.Name}@{tab.TellTarget?.World}"
);
@@ -519,13 +573,18 @@ internal sealed class AutoTellTabsService : IDisposable
// If the unpinned pool is already full, dropping the oldest before
// flipping the flag avoids counting the just-unpinned tab as a drop
// candidate.
if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit)
// candidate. Under lock, since DropOldestTempTab mutates the list.
// SaveConfig stays outside, see TryPin.
lock (TabsListLock)
{
DropOldestTempTab();
if (ActiveTempTabCount >= Plugin.Config.AutoTellTabsLimit)
{
DropOldestTempTab();
}
tab.IsPinned = false;
}
tab.IsPinned = false;
_logger.LogDebug("[Pin] Unpinned tab '{TabName}'", tab.Name);
_plugin.SaveConfig();
}
@@ -542,7 +601,12 @@ internal sealed class AutoTellTabsService : IDisposable
// see StripTellBindingOnPromote; clearing Tab.TellTarget alone would leave
// CurrentChannel.Channel == Tell + a stale target and route a typed line
// silently as /tell to the old partner.
TabLifecycleHelpers.StripTellBindingOnPromote(tab);
// Flips IsTempTab/IsPinned, which decide pool membership and whether a save
// strips the tab. Under lock so a concurrent save sees one or the other, never
// half. SaveConfig stays outside, see TryPin.
lock (TabsListLock)
TabLifecycleHelpers.StripTellBindingOnPromote(tab);
_logger.LogDebug($"[Pin] Promoted tab '{tab.Name}' to permanent (tell-binding dropped)");
_plugin.SaveConfig();
}