Repository navigation
Clock and timer fixes from a review of every timer call site - #403
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe changes revise timer semantics and time-source usage across common, UI, and viewer code. They also correct timestamp calculations, update reflection-probe detachment and scheduling, and adjust fatal-signal handling and frame-processing behavior. ChangesTiming and Runtime Behavior
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed timing changes preserve their clock origins or correct the GPU-time scale, and retry expirations use a consistent deadline. No concrete user-facing regression is established, so the PR appears ready to merge subject to normal project checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the timers tick, Comment |
RyeMutt
force-pushed
the
rye/clock-fixes
branch
2 times, most recently
from
October 5, 2026 03:36
3e4b866 to
09a85a9
Compare
…arts onError pumps mainloop for up to two seconds so the error reaches the plugin. The deadline came from LLTimer::getElapsedSeconds(), which reads the global timer and returns 0 while it does not exist: before LLCommon::initClass and after cleanupClass. An error logged then made the deadline 2 and the clock 0, and the loop spun for as long as the plugin's stdin stayed full. A local LLTimer starts its own clock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The horizon was converted to clock ticks in the initialiser list, using a frequency that is 0 until something first reads the clock. A static LLDeadmanTimer, such as LLMeshRepository's quiescent timer, constructed before that got a horizon of 0 and expired at once. The constructor now reads the frequency first, as LLTimer's does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the reply arrived in the same message-time sample the ping was sent in, the elapsed time read 0. The code refreshed the sample to get a real time, then went on using the 0 it had already computed, so those pings were recorded as about 0 ms and pulled the averaged ping down. The elapsed time is now taken again from the refreshed sample. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…2080 processExperience adds the current time to EXPIRES, because the server sends it as a delay. The error path built its placeholder rows with the current time already added, so the expiry came out at about twice the epoch. A failed lookup was never retried for the session, and an existing entry whose refresh failed was written to the cache file with that expiry and never refreshed in later sessions either. The error path now stores the delay, as the error_ids path does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLFrameTimer::setTimerExpirySec counts from the timer's last reset, and mTripleClick was never reset, so arming it after a double click set an expiry 0.3 s after the view was constructed. Once a view was older than that, every third press read as expired and triple-click line selection never worked. Arming now resets the timer with its expiry. The test lets the triple-click window pass after the view is made, then double-clicks and presses again; it fails with the old arming. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
documentRead set mRereadAt's expiry without resetting it, and an LLFrameTimer counts its expiry from its last reset, which for this timer was the floater's construction. Once the floater was 0.75 s old every edit was already past the expiry, so the lint, the tree suffixes and the findings ran again on the frame after each keystroke instead of once the typing paused. It now resets first, as the source-edit timer beside it does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
message_time.getElapsedSeconds() is LLTimer's static uptime, called through an instance, so the "response arrived too early" check compared the viewer's uptime against 10 seconds and was dead after the first ten seconds of a session. A 499 or 5xx that came straight back was treated as an ordinary empty poll: reposted at once, with no backoff, and the error count reset. It now measures the request, so a fast failure takes the error path and its backoff, as the code intends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLTimer::start() resets the timer, and reset() clears the expiry, so setting the expiry and then starting left both the task timer and the batch timer already expired. The first checkTimeout() of every non-priority update suspended until the next frame. Starting first keeps the expiry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
draw() set the throttle's expiry and then called start(), and LLFrameTimer::start() resets the expiry to now, so the timer had expired again by the next frame and onList() ran on every draw. resetWithExpiry sets both at once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
setFetching(FETCH_FAILED) set the expiry without a reset, and an LLFrameTimer counts its expiry from its last reset, which was the start of the fetch. A fetch that failed after more than 60 seconds, and AIS times out at 180, had its back-off expire before it began, so the folder was fetched again at once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Access times were epoch seconds cast to F32, which at the current epoch resolves 128 seconds. The LRU could not order groups touched within the same 128 s, and because it only evicts a group strictly older than now, none touched in the current bucket could go: the eviction loop gave up and the cache grew past MAX_CACHED_GROUPS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getArrivalTimeByID returned epoch seconds as F32, which resolves 128 seconds at the current epoch, so everyone who arrived within the same two minutes compared equal and the list fell back to sorting them by name. The map already held F64; the getter and the comparator now do too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLVOAvatar::getTotalGPURenderTime() is in milliseconds, but the floater converted it with us_to_raw, so the avatar share came out a thousand times too small and the scenery share, which subtracts it, too large. The auto-tuner reads the same value with ms_to_raw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
destroySavedRawImage compared the time of the last reference, an absolute time since startup, against the keep time, a duration. A keep time of 30 seconds, as the bump maps ask for, protected the image only during the first 30 seconds of the session. It now compares the time since the last reference. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
send_agent_pause() sets mWasPaused so the stalled frame is left out of the frame statistics, but nothing ever cleared it. After the first modal file or directory picker, frame time, jitter, the percentiles and the normalised variance stopped updating for the rest of the session. The flag is now cleared once the skipped frame has been passed over. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
createObjects got 5% of the last frame interval with no upper bound, unlike the decode budgets beside it. The interval is measured between object-list updates, which stop during a teleport, so the first frame after a 20 second teleport could spend a second creating objects. It now stops at 5 ms, the decode budget's ceiling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getMultipleOpenFiles, getSaveFile and getDir called LLFrameTimer::updateFrameTime() whatever the mode. On Windows they run on LLFilePickerThread and LLDirPickerThread with blocking off, so a worker thread rewrote the frame clock while the main thread was in the middle of a frame. Only the modal case stalls the app, and only it now updates the clock, as getOpenFile already did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gRenderStartTime and gForegroundTime are reset when the scene starts loading, but what is divided by them was not: - sim_fps used a last time taken before the reset, so its first denominator was the time since the reset minus the time to the end of init, which can be zero or negative. - fps divided frames counted since the app started by foreground time since the reset, so it included every login-screen frame. - Resetting gForegroundTime while it was paused, which it is when the window is unfocused at that moment, stored an absolute time where the paused elapsed time belongs; the next unpause turned it into seconds since startup. The counters now reset with their timers, and the foreground timer is reset running and paused again if it was paused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 0.8 second window that merges rapid changes into one undo step was measured on system_clock, so a backward step of the wall clock put every change inside the window and no undo snapshots were taken until the clock caught up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The five second pending window was kept in whole seconds of time(), the wall clock, so a request could expire up to a second early, and a backward clock step held every request for that profile as pending for the length of the step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
isOlderThan subtracted the stored time from now in unsigned seconds, so a conversation whose time was ahead of the corrected clock wrapped to a huge age and was purged from the log at login. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Losing focus stops the open timer and starts the fade; moving the mouse out then unpaused the stopped timer. unpause() turns a paused timer's stored elapsed time back into a start time, but a stopped one holds its start time instead, so the inspector read as long past its stay time and started the fade again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
These compared a time the server stamped against this machine's clock, which is off by however far the local clock is wrong: - event reminders (lleventnotifier); - the remaining time of parcel access and ban entries (llfloaterland); - the expiry sent with a timed parcel ban (llfloaterbanduration); - the display-name change lockout (llfloaterdisplayname, llpanelprofile); - which chat-log date is today (lllogchat), which has to agree with the time_corrected() stamps it is matched against. All now use time_corrected(), the clock corrected to the server's at login. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The day cycle's position, its blend timing and the environment panel's apparent time were taken from this machine's clock, so a viewer whose clock was wrong showed a different time of day from the region and from everyone else. They now add gUTCOffset, the offset to the server's clock measured at login. The per-frame sites keep LLDate::now()'s sub-second precision; time_corrected() is whole seconds and would make the sky step once a second. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getSaveFile reset gKeyboard after the dialog closed whatever the mode, and on Windows the non-modal case runs on LLFilePickerThread, so a worker thread rewrote the key state while the main thread was reading it. That reset is only needed when the dialog is modal and the main thread was blocked in it; threaded, the dialog taking focus already resets the keyboard on the main thread through handleFocusLost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLTimer::getElapsedSeconds() and LLFrameTimer::getElapsedSeconds() are statics returning the application's uptime, but read like the elapsed time of the timer they are called on, and C++ lets them be called through one. The event poll's early-reply guard did exactly that and measured the uptime instead of its request. Both are now getUptimeSeconds(), and every caller was read again on the way. The debug infinite loop called the static through an instance too, and now prints its own timer's elapsed time. LLVoiceVisualizer kept an LLFrameTimer member only to call the static getTotalSeconds() through it; it calls the static directly and the member is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLFrameTimer::setTimerExpirySec counted from the timer's last reset and LLTimer::setTimerExpirySec counts from now. Code that set an expiry on an LLFrameTimer without resetting it first got an expiry measured from whenever the timer was last reset, often its construction; that is where the broken triple-click, the XUI Studio reread, the avatar picker throttle and the inventory back-off came from. Every LLFrameTimer caller in the tree was read: each resets or starts the timer immediately before setting the expiry, so none changes. What changes is that the next caller to forget the reset gets the expiry it asked for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A paused LLFrameTimer holds its elapsed time where a running one holds its start time. stop() and reset() did not keep to that: stop() only cleared the started flag, leaving the start time behind, and reset() wrote a start time whatever the state. A stopped timer, or one reset while paused, then read its absolute start time back as its elapsed time, and a later unpause() turned that into nonsense. stop() now freezes the time run as pause() does, reset() zeroes the elapsed time of a paused timer and leaves it paused, and start() marks the timer running before it resets. setExpiryAt, setAge and getElapsedTimeAndResetF32 keep to the same rule. The scene-load telemetry no longer has to unpause gForegroundTime around its reset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLTimer::start() and reset() clear the expiry, so setting one and then starting the timer leaves it expired, which is how the AIS update timers went wrong. resetWithExpiry does both in the order that works, as LLFrameTimer's already did. The new test pins that a fresh timer reads expired, that resetWithExpiry keeps its expiry, that start() clears one, and that the uptime is the application's and not the timer's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A live file's event timer fires once its period has passed on a live clock, and the check it called then asked a frame-quantised timer whether the same period had passed. Frame time could fall short of the period by part of a frame, the check was skipped, and the file was next looked at a whole period later: up to twice the refresh period for logcontrol.xml, fonts.xml and externally edited notecards. The event timer now forces the check, since it has already waited the period. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without a crash reporter the viewer installs its own handler, and it answered a fatal signal on the main thread by calling LLApp::setError() from inside the handler. That posts the status change, and its listeners close work queues and join thread pools: none of it safe in a signal handler, and after a fault the heap may already be damaged. Each worker the join wakes frees its malloc cache on the way out and aborts on that damage. A Linux crash in LLReflectionMap::syncToViewerObject came back as a worker's munmap_chunk() SIGABRT, with a third thread terminating on a fiber mutex lock_error inside a re-entered handler, and the fault that started it buried under both. Every fatal signal now restores the default handlers and re-raises, as helper threads already did. The isError() check for a second signal goes, since it locks a fiber mutex and the restored defaults already make a second signal fatal. So does the smackdown branch's setDefaultLevel call, which only quietened the shutdown that no longer runs, and the --disablecrashlogger branch folds in, having done exactly this already. The cost is the final "status: error" log line; none of the LLApp listeners reports crashes. The handler builds only on Linux and macOS and was checked by reading; llapp.cpp compiles on Windows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLReflectionMap::mGroup is a raw pointer, and ~LLSpatialGroup left it set. The manager keeps a probe until its next update finds nothing else holds it, and in that window autoAdjustOrigin dereferences mGroup and runs lineSegmentIntersect through it. Clearing the pointer alone would make such a probe look like a terrain probe, which is all a probe with neither a group nor a viewer object is, and a manual probe already did after markDead: relevant at RenderReflectionProbeLevel 2, so free to take a cube slot, join neighbour lists and draw an occlusion query in the pass before it was deleted. Every owner now calls LLReflectionMap::orphan() when it lets go -- the spatial group's destructor, LLViewerObject::markDead and its destructor, LLVOVolume when an object stops being a probe, and the region's destructor for its terrain probes. orphan() clears the pointer back and marks the probe, and isRelevant() rejects an orphan before it looks at which pointers are set. An object that stops being a probe now drops its probe's reference to it at once; that probe used to stay a live manual probe for one more pass. Compiled on Windows; runtime verification owed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The probe being updated was finished whatever became of it, so each of its remaining passes rendered the whole scene for a probe the release pass was about to take the slot from, or, once orphaned, to delete. update() now abandons it first, by the rule the scheduling loop uses to skip a probe: the default probe is exempt and pausing does not count. That also covers coverage being lowered mid-update, where at level 0 a non-default probe could reach the llassert in updateProbeFace, and an automatic probe a manual probe swallows partway through. deleteProbe already abandoned an update when it deleted the probe being updated, but left mRadiancePass set. The radiance half runs second and is what marks a probe complete, so the next probe started on it, skipped its irradiance projection and was marked complete on whatever SH coefficients its slot's previous owner left behind. Both paths now go through abandonProbeUpdate(), which resets the probe, the face and the pass together. Compiled on Windows; runtime verification owed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLInspectAvatar and LLInspectObject override onMouseLeave and ended it with their own unconditional mOpenTimer.unpause(), so the guard 9c10a3d put in LLInspect::onMouseLeave never reached them, and they are the inspectors people see most. Since d83cb8b a stopped timer keeps the time it ran, so the unpause resumed it: an inspector that lost focus after two seconds and was crossed by the mouse during its fade snapped back to full opacity for the second it had left before fading again. Both now finish through the base, which unpauses only while the fade has not begun. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The scene-load start resets gForegroundTime, and since d83cb8b a reset while paused leaves it at no time run rather than the uptime. A viewer that has not had focus since then, alt-tabbed away during login, sends stats with no foreground time and no foreground frames, and the fps divided one by the other into a NaN in the ViewerStats body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9513d15 cleared mWasPaused after the first update following a pause, so the stats stopped skipping one frame in, however long the pause lasted. Minimised, every throttled background frame went into the frame time, jitter and percentile stats while the recording they belong to was stopped. The flag now follows the pause until send_agent_resume, and the frame after the resume, which the pause stalled, is still skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The probe sort orders by priority before distance, so the FLT_MAX distance update() gives an irrelevant probe put an orphaned manual probe last among the manual ones, still ahead of every automatic probe. For the update before it was deleted it took a place inside the budget and pushed the last automatic probe past it, and the release pass took that probe's cube slot, so it faded out and regenerated all twelve passes. orphan() now drops the priority to 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
09a85a9 checked the updating probe's relevance before the eclipse pass recomputes mInsideManualProbe, so it judged on last frame's. A probe whose eclipsing manual probe had just gone was abandoned, found relevant again a few lines later, kept its slot and started its twelve passes over. The check, and the update after it, now follow the eclipse pass, which also has the probe track its viewer object before the face is rendered. cleanup() reset the probe and the face by hand but not mRadiancePass. It was harmless, since initReflectionMaps clears the flag after a cleanup, but it now goes through abandonProbeUpdate() like every other path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getErrorRetryDeltaTime's comment still called its result seconds since the epoch, the reading b9ae6b1 removed, and the one that would bring back the expiry around 2080 if a caller trusted it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLTimer's constructor and, since eb5dbf9, LLDeadmanTimer's each checked for a frequency of 0 and computed it, so a timer built during static initialisation did not scale by 0. TimerInfo now computes it when it is constructed, on the first get_timer_info(), so every reader has it and neither constructor needs its own copy of the check. LLDeadmanTimer's constructor is back to what it was before eb5dbf9. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RyeMutt
force-pushed
the
rye/clock-fixes
branch
from
October 6, 2026 20:09
09a85a9 to
1d5f458
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from a review of every clock and timer call site in the tree, plus three probe and signal-handler fixes that landed on the same branch.
The expiry trap
LLFrameTimer::setTimerExpirySeccounted from the timer's last reset, whileLLTimer's counts from now, andLLTimer::start()/reset()clear the expiry. Between them:ALTextViewnever worked.The event poll's "response arrived too early" guard called the static uptime getter through an instance, so it was dead after ten seconds of uptime. Linden's code has the same bug. Fast 499/5xx replies now take the error path with its backoff, and fifteen in a row force a disconnect, as the code intends.
The API underneath is fixed too:
LLFrameTimerexpiries count from now. Every existing caller was read first, and none changes behaviour.LLFrameTimerkeeps its elapsed time, instead of reading back its absolute start time.LLTimer::resetWithExpiryis added.getUptimeSeconds.Precision and units
F64. AsF32they resolved 128 s, so the group cache grew past its cap.Threads and stats
sim_fps, foreground fps) counts from the scene-load start.LLLeap's error drain no longer spins forever when the global timer doesn't exist.Server time
Smaller fixes
Also on this branch
88f684d250: fatal signals restore the default handlers and re-raise, instead of callingLLApp::setError()from inside the handler. Linux and macOS only; checked by reading, and it compiles on Windows.155bfbd209,d165e55ba3: reflection probes are orphaned when their owner lets go of them, and a probe update stops once its probe is no longer relevant. Compiled on Windows; runtime verification is owed.Testing
Built RelWithDebInfo on Windows. 375/375 ctest at
fa9a934c9d, before the last three commits.New tests:
altextviewtest 78 (triple-click);llframetimertests 4–6 (expiry from now,stop(),reset()while paused);lltimer_test.Each new test was run against the old code to confirm it fails.
The clock and timer commits were verified in the viewer on Windows.
🤖 Generated with Claude Code