perf(social): cap feed length to stop long browsing sessions going ch… - #44
perf(social): cap feed length to stop long browsing sessions going ch…#44Kiro-XVI wants to merge 1 commit into
Conversation
XeldarAlz
left a comment
There was a problem hiding this comment.
Built the branch head clean, 42/42 tests pass. Not run in-game. The branch also conflicts with master now (DrawFeedList has moved in both apps), so it needs a rebase.
One blocking finding inline, the rest are non-blocking. The analysis in the description is accurate: I confirmed the lanes really are unbounded for the process lifetime, and that RemoteImageCache (LRU-budgeted at 160MB) and RichTextCache (capped at 256) are correctly ruled out.
|
|
||
| public const int FeedCap = 300; | ||
|
|
||
| public void TrimFeed(SocialFeedScope scope, int max) => Lane(scope).Trim(max); |
There was a problem hiding this comment.
Blocking: Velvet is a third social feed with the same problem and it is not covered here. It has its own VelvetStore holding FeedLane<VelvetPostDto>[], and the same uncapped draw loop at VelvetShell.Feed.cs:68, so it keeps growing all day after this merges. The description is right that Chirper comes along for free, but Velvet does not.
|
|
||
| public bool LoadingMore(SocialFeedScope scope) => Lane(scope).LoadingMore; | ||
|
|
||
| public const int FeedCap = 300; |
There was a problem hiding this comment.
Non-blocking: the cap constant lives here, but the policy that decides when to apply it (the scroll check) lives in each app's draw method, so a new social feed opts out just by forgetting a line. FeedVirtualizer is the seam all three feeds already share: give BeginFrame an optional trimmable source and the cap, the scroll guard and Velvet coverage all land in one place.
| } | ||
| } | ||
|
|
||
| public void Trim(int max) |
There was a problem hiding this comment.
Non-blocking, answering the open question: option 1 is the right call and option 3 does not work. Per-page cursors cannot rewind a trim, because refreshes prepend to the head, so a recorded page boundary drifts deeper over the day. In the issue's own scenario (head growth from repeated app opens) the page-1 boundary ends up past the cap, and a boundary-aligned trim either refuses to fire or rewinds to a cursor from hours ago. A real rewind needs a cursor the client can derive from a post id or timestamp, which is a server change.
|
|
||
| lock (gate) | ||
| { | ||
| if (items.Length <= max) |
There was a problem hiding this comment.
Non-blocking: please add coverage to FeedLaneTests.cs. Trim is pure and needs no ImGui, so keep-newest, under-cap no-op and cursor-survives-trim are three cheap tests.
| } | ||
|
|
||
| var trimmed = new TPost[max]; | ||
| Array.Copy(items, trimmed, max); |
There was a problem hiding this comment.
Non-blocking: Trim reads and writes items under gate, but ReplacePost, ApplySavedEverywhere and RemoveAuthorEverywhere all assign lane.Items without it. A background update landing between the read and the write here is silently dropped, so a like or follow flag can revert until the next refresh. Pre-existing pattern rather than something you introduced, just widened by one more writer.
| var snapshot = store.Feed(scope); | ||
| using (AppSurface.Begin(listRect)) | ||
| { | ||
| if (ImGui.GetScrollY() < 1f) |
There was a problem hiding this comment.
Non-blocking: OnOpened does not reset feed scroll (only RefreshActiveFeed sets feedScrollTopPending) and ImGui keeps the child's scroll offset, so a user who leaves the feed scrolled down can go a long time without ever satisfying this guard. That is exactly the browse-all-day case in the issue, so the mitigation may fire less often than expected.
| if (ImGui.GetScrollY() < 1f) | ||
| { | ||
| store.TrimFeed(scope, SocialFeedStore.FeedCap); | ||
|
|
There was a problem hiding this comment.
Nit: stray blank line inside the block.
Fixes #43.
Cause
FeedLane.itemsonly ever grows —ApplyRefreshmerges new posts in at the head,ApplyMoreappends older ones at the tail, and nothing is ever removed.Memory isn't really the issue.
DrawFeedListloops over every loaded post each frame and callsFeedVirtualizer.Skipon each one. Even when culling works, that's two ImGui cursor calls plus a dictionary lookup per post per frame — a couple of thousand posts at 60fps is a lot of work to step over invisible rows.FeedVirtualizer.BeginFramealso clears its whole height cache when content width or font generation changes. Nothing can be skipped on the frame after that clear, so everything loaded gets drawn at once — probably the more visible hitch.RichTextCachewas the other suspect but it's already capped at 256 and clears itself, so it isn't leaking.Change
FeedLane.Trim(max)keeps the newestmaxitems (the lane is sorted newest-first).SocialFeedStoreexposesTrimFeed(scope, max)and aFeedCapconstant; both apps call it fromDrawFeedList, guarded onImGui.GetScrollY() < 1f.The scroll guard matters — trimming only when the user is parked at the top means everything removed is far below the viewport, so there's no jolt. Trimming inside
ApplyMorewould be wrong: items are newest-first, so it would immediately drop the page infinite scroll had just fetched.Both apps go through
SocialFeedStore/FeedLane, so Chirper is covered by the same change.Constraint worth knowing
The cap must sit comfortably above the server's page size. Testing at 5 produced a visible loop: trim to 5 → next refresh merges a full page → back over the cap → trim again, with the scrollbar flashing each cycle. 300 is well clear of this.
Open question
cursoris a single value pointing past the oldest post loaded, and there's no way to rewind it. So after a trim, scrolling back down far enough fetches from the cursor and leaves a gap where the trimmed posts were.Options as I see them:
Happy to do 3 instead if you'd prefer — it touches paging more broadly, so it felt like your call rather than mine.
Testing
CachyOS, wine-xiv-staging-fsync-git 10.8, Dalamud 15.0.2.3. Verified with a temporarily lowered cap: trim fires on returning to the top, infinite scroll still fetches afterwards, both apps behave the same.