perf(user_status): skip heartbeats another tab already sent - #64137
perf(user_status): skip heartbeats another tab already sent#64137pringelmann wants to merge 1 commit into
Conversation
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
|
/backport to stable35 |
| const announce = (force = false) => { | ||
| // NaN (missing or unparseable) and a negative age (future timestamp) | ||
| // both fail this test, so both send | ||
| const age = Date.now() - Number.parseInt(browserStorage.getItem('lastHeartbeat') ?? '', 10) | ||
| if (!force && age >= 0 && age < HEARTBEAT_THROTTLE) { | ||
| return | ||
| } | ||
| browserStorage.setItem('lastHeartbeat', String(Date.now())) | ||
| beat(isAway) | ||
| } |
There was a problem hiding this comment.
Settings, reading and parsing browser storage seems to be a bit overkill here (if this is done sync it is quite slow as well).
Why not using BroadcastChannel instead?
There was a problem hiding this comment.
Why not using BroadcastChannel instead?
BroadcastChannel only reaches tabs that are alive/active, and most of the win here is surviving a page load. Since navigations are full reloads, a fresh page can't learn that the previous page reported a heartbeat 10 seconds ago. A braodcast would only cover multi-tab, and we would need some kind of ownership election. (quite a lot more complex to implement than what I have added here)
if this is done sync it is quite slow as well
On the sync cost: agreed in general, but the expensive part of localStorage is the first access loading the storage area, and this is not the first code path that touches it on page load. Core and notifications both read/write it on every logged-in page. And this runs once per page load plus once per 5 minutes, so it's not a particularly hot code path.
Also worth pointing out that notifications already does the same thing btw: it throttles its cross-tab fetches with a lastUpdated timestamp in browser storage.
There was a problem hiding this comment.
@susnux does the reasoning above work for you? Would like to get this in for today's RC, and it needs a second approval.
If you'd still rather have BroadcastChannel I'll happily do it as a follow-up.
There was a problem hiding this comment.
No makes sense, I honestly was not aware it does not work with not-active tabs.
3 participants
Summary
Every tab and every page load announced presence on its own, so the dashboard sent two heartbeats (it mounts two instances) and hopping between apps sent one per page.
They now share a
lastHeartbeattimestamp in browser storage and send at most one announcement per four minutes, well inside the server's fifteen minute staleness threshold. A tab opened in the background waits until it's actually looked at.The five minute interval is longer than the four minute window, so a single tab keeps its existing cadence and the gap to the server never grows. Coming back from away is never throttled.
Checklist
3. to review, feature component)stable32)AI (if applicable)