Clock and timer fixes from a review of every timer call site

indra/llaudio/llaudioengine.cpp

@@ -1419,7 +1419,7 @@ void LLAudioEngine::logSoundPlay(const LLUUID& id, LLVector3d position, S32 type
     item->mOwnerID = ownerid;
     item->mSourceID = sourceid;
     item->mPlaying = true;
-    item->mTimeStarted = LLTimer::getElapsedSeconds();
+    item->mTimeStarted = LLTimer::getUptimeSeconds();
     item->mTimeStopped = F64_MAX;
     item->mIsTrigger = is_trigger;
     item->mIsLooped = is_looped;
@@ -1437,7 +1437,7 @@ void LLAudioEngine::logSoundStop(const LLUUID& id)
     {
         LLSoundHistoryItem& hist_item = *iter->second;
         hist_item.mPlaying = false;
-        hist_item.mTimeStopped = LLTimer::getElapsedSeconds();
+        hist_item.mTimeStopped = LLTimer::getUptimeSeconds();
         pruneSoundLog();
     }
 }

indra/llcommon/CMakeLists.txt

@@ -340,6 +340,7 @@ if(AL_BUILD_TESTS)
     llstreamtools
     llstring
     llstring_utf
+    lltimer
     lltrace
     lltreeiterators
     llunits

indra/llcommon/llapp.cpp

@@ -38,13 +38,11 @@
 
 #include "llcommon.h"
 
-#include "llerrorcontrol.h"
 #include "llframetimer.h"
 #include "lllivefile.h"
 #include "llmemory.h"
 #include "llstl.h" // for DeletePointer()
 #include "llstring.h"
-#include "llthread.h" // for on_main_thread()
 #include "lleventtimer.h"
 #include "stringize.h"
 #include "llcleanup.h"
@@ -377,10 +375,9 @@ void LLApp::sendOutOfDiskSpaceNotification()
 #ifndef LL_WINDOWS
 // With a crash reporter in the process, the crash signals are its own:
 // crashpad on Linux and SentryCrash on macOS install their handlers after
-// this runs, and a handler of ours underneath would run the app's shutdown
-// from inside a crashed process once the reporter re-raises. The signals the
-// app answers itself, for a graceful quit or to ignore, stay with it either
-// way.
+// this runs, and a handler of ours underneath would have nothing to add once
+// the reporter re-raises. The signals the app answers itself, for a graceful
+// quit or to ignore, stay with it either way.
 void setup_signals()
 {
     //
@@ -484,8 +481,6 @@ void default_unix_signal_handler(int signum, siginfo_t *info, void *)
 {
     // Unix implementation of synchronous signal handler
     // This runs in the thread that threw the signal.
-    // We do the somewhat sketchy operation of blocking in here until the error handler
-    // has gracefully stopped the app.
 
     // FIXME(brad) - we are using this handler for asynchronous signals as well, so sLogInSignal is currently
     // disabled for safety.  we need to find a way to selectively reenable it when it is safe.
@@ -555,66 +550,28 @@ void default_unix_signal_handler(int signum, siginfo_t *info, void *)
                 {
                     LL_WARNS() << "Signal handler - Handling smackdown signal!" << LL_ENDL;
                 }
-                else
-                {
-                    // Don't log anything, even errors - this is because this signal could happen anywhere.
-                    LLError::setDefaultLevel(LLError::LEVEL_NONE);
-                }
 
                 // Change the signal that we reraise to SIGABRT, so we generate a core dump.
                 signum = SIGABRT;
             }
 
             if (LLApp::sLogInSignal)
             {
-                LL_WARNS() << "Signal handler - Handling fatal signal!" << LL_ENDL;
-            }
-
-            if (LLApp::isError())
-            {
-                // Received second fatal signal while handling first, just die right now
-                // Set the signal handlers back to default before handling the signal - this makes the next signal wipe out the app.
-                clear_signals();
-
-                if (LLApp::sLogInSignal)
-                {
-                    LL_WARNS() << "Signal handler - Got another fatal signal while in the error handler, die now!" << LL_ENDL;
-                }
-                raise(signum);
-                return;
+                LL_WARNS() << "Signal handler - Fatal signal, passing it back to the operating system" << LL_ENDL;
             }
 
-            if (LLApp::sLogInSignal)
-            {
-                LL_WARNS() << "Signal handler - Flagging error status and waiting for shutdown" << LL_ENDL;
-            }
-
-            if (LLApp::isCrashloggerDisabled()) // Don't gracefully handle any signal, crash and core for a gdb post mortem
-            {
-                clear_signals();
-                LL_WARNS() << "Fatal signal received, not handling the crash here, passing back to operating system" << LL_ENDL;
-                raise(signum);
-                return;
-            }
-
-            if (!on_main_thread())
-            {
-                // A fatal signal on a helper thread cannot be handled gracefully:
-                // setError() posts events and joins thread pools, which deadlocks
-                // if the crashing thread is one the shutdown depends on, e.g. a
-                // libdispatch worker inside the macOS GL driver. Crash instead.
-                clear_signals();
-                raise(signum);
-                return;
-            }
-
-            // Flag status to ERROR
-            LLApp::setError();
-
-            if (LLApp::sLogInSignal)
-            {
-                LL_WARNS() << "Signal handler - App is stopped, reraising signal" << LL_ENDL;
-            }
+            // A fatal signal is never handled here, on any thread. LLApp::setError()
+            // posts the status change, and its listeners close work queues and join
+            // thread pools -- none of it safe inside a signal handler, least of all
+            // after a fault, when the heap may already be damaged. Each worker that
+            // exits frees its malloc cache on the way out and aborts on that damage,
+            // so the core records a worker's abort instead of the fault that started
+            // it; and a fault on a thread the shutdown waits for, such as a
+            // libdispatch worker inside the macOS GL driver, deadlocks outright.
+            // LLApp::isError() is no safer, since it locks a fiber mutex.
+            //
+            // Restoring the default handlers before re-raising also covers a second
+            // fatal signal: it takes the process down by default.
             clear_signals();
             raise(signum);
             return;

indra/llcommon/llframetimer.cpp

@@ -51,18 +51,22 @@ void LLFrameTimer::updateFrameTime()
 
 void LLFrameTimer::start()
 {
-    reset();
     mStarted = true;
+    reset();
 }
 
 void LLFrameTimer::stop()
 {
-    mStarted = false;
+    // Frozen at the time run so far, as pause() leaves it: a stopped timer's
+    // elapsed time is otherwise read back as its absolute start time.
+    pause();
 }
 
 void LLFrameTimer::reset()
 {
-    mStartTime = sFrameTime;
+    // A paused timer keeps its elapsed time where a running one keeps its
+    // start, so a reset while paused zeroes that rather than writing a start.
+    mStartTime = mStarted ? sFrameTime : 0.0;
     mExpiry = sFrameTime;
 }
 
@@ -96,12 +100,12 @@ void LLFrameTimer::unpause()
 
 void LLFrameTimer::setTimerExpirySec(F32 expiration)
 {
-    mExpiry = expiration + mStartTime;
+    mExpiry = expiration + sFrameTime;
 }
 
 void LLFrameTimer::setExpiryAt(F64 seconds_since_epoch)
 {
-    mStartTime = sFrameTime;
+    reset();
     mExpiry = seconds_since_epoch - (USEC_TO_SEC_F64 * sStartTotalTime);
 }
 

indra/llcommon/llframetimer.h

@@ -42,10 +42,10 @@ class LL_COMMON_API LLFrameTimer
     LLFrameTimer() : mStartTime( sFrameTime ), mExpiry(0), mStarted(true) {}
 
     // Return the number of seconds since the start of this
-    // application instance.
-    static F64SecondsImplicit getElapsedSeconds()
+    // application instance, as of the current frame. Static: it is not this
+    // timer's elapsed time, which is getElapsedTimeF32().
+    static F64SecondsImplicit getUptimeSeconds()
     {
-        // Loses msec precision after ~4.5 hours...
         return sFrameTime;
     }
 
@@ -82,12 +82,12 @@ class LL_COMMON_API LLFrameTimer
     void resetWithExpiry(F32 expiration);
     void pause();
     void unpause();
-    void setTimerExpirySec(F32 expiration);
+    void setTimerExpirySec(F32 expiration);         // Expires this long from now, as LLTimer's does
     void setExpiryAt(F64 seconds_since_epoch);
     bool checkExpirationAndReset(F32 expiration);
-    F32 getElapsedTimeAndResetF32()                 { F32 t = F32(sFrameTime - mStartTime); reset(); return t; }
+    F32 getElapsedTimeAndResetF32()                 { F32 t = getElapsedTimeF32(); reset(); return t; }
 
-    void setAge(const F64 age)                      { mStartTime = sFrameTime - age; }
+    void setAge(const F64 age)                      { mStartTime = mStarted ? sFrameTime - age : age; }
 
     // ACCESSORS
     bool hasExpired() const                         { return (sFrameTime >= mExpiry); }

indra/llcommon/llleap.cpp

@@ -442,8 +442,8 @@ class LLLeapImpl: public LLLeap
             LLProcess::WritePipe& childin(mChild->getWritePipe(LLProcess::STDIN));
             LLEventPump& mainloop(LLEventPumps::instance().obtain("mainloop"));
             LLSD nop;
-            F64 until = (LLTimer::getElapsedSeconds() + 2).value();
-            while (childin.size() && LLTimer::getElapsedSeconds() < until)
+            LLTimer waited;
+            while (childin.size() && waited.getElapsedTimeF64() < 2.0)
             {
                 mainloop.post(nop);
             }

indra/llcommon/lllivefile.cpp

@@ -161,30 +161,16 @@ std::string LLLiveFile::filename() const
     return impl.mFilename;
 }
 
-namespace
-{
-    class LiveFileEventTimer : public LLEventTimer
-    {
-    public:
-        LiveFileEventTimer(LLLiveFile& f, F32 refresh)
-            : LLEventTimer(refresh), mLiveFile(f)
-            { }
-
-        bool tick()
-        {
-            mLiveFile.checkAndReload();
-            return false;
-        }
-
-    private:
-        LLLiveFile& mLiveFile;
-    };
-
-}
-
 void LLLiveFile::addToEventTimer()
 {
-    impl.mEventTimer = new LiveFileEventTimer(*this, impl.mRefreshPeriod);
+    // The event timer keeps the refresh period itself, so each tick checks.
+    // Asking the frame-quantised gate again could find a frame less than a
+    // period since the last check and skip one, doubling the period.
+    impl.mEventTimer = LLEventTimer::run_every(impl.mRefreshPeriod, [this]()
+        {
+            impl.mForceCheck = true;
+            checkAndReload();
+        });
 }
 
 void LLLiveFile::setRefreshPeriod(F32 seconds)

indra/llcommon/lltimer.cpp

@@ -254,7 +254,11 @@ TimerInfo::TimerInfo()
 :   mClockFrequency(0.0),
     mTotalTimeClockCount(0),
     mLastTotalTimeClockCount(0)
-{}
+{
+    // Known from the first read, so a timer constructed during static initialisation, before
+    // anything else has touched the clock, never scales by a frequency of 0.
+    update();
+}
 
 void TimerInfo::update()
 {
@@ -319,11 +323,6 @@ U64MicrosecondsImplicit totalTime()
 
 LLTimer::LLTimer()
 {
-    if (!get_timer_info().mClockFrequency)
-    {
-        get_timer_info().update();
-    }
-
     mStarted = true;
     reset();
 }

indra/llcommon/lltimer.h

@@ -66,8 +66,9 @@ class LL_COMMON_API LLTimer
     static void cleanupClass();
 
     // Return a high precision number of seconds since the start of
-    // this application instance.
-    static F64SecondsImplicit getElapsedSeconds()
+    // this application instance. Static: it is not this timer's elapsed
+    // time, which is getElapsedTimeF64().
+    static F64SecondsImplicit getUptimeSeconds()
     {
         if (sTimer)
     {
@@ -91,7 +92,8 @@ class LL_COMMON_API LLTimer
     void stop() { mStarted = false; }
     void reset();                               // Resets the timer
     void setLastClockCount(U64 current_count);      // Sets the timer so that the next elapsed call will be relative to this time
-    void setTimerExpirySec(F32SecondsImplicit expiration);
+    void setTimerExpirySec(F32SecondsImplicit expiration);      // Expires this long from now
+    void resetWithExpiry(F32SecondsImplicit expiration) { reset(); setTimerExpirySec(expiration); }
     bool checkExpirationAndReset(F32 expiration);
     bool hasExpired() const;
     F32SecondsImplicit getElapsedTimeAndResetF32(); // Returns elapsed time in seconds with reset

indra/llcommon/tests/llframetimer_test.cpp

@@ -112,10 +112,65 @@ namespace tut
         ensure("timer took too long to expire", iterations_until_expiration <= 10);
     }
 
-/*
     template<> template<>
     void frametimer_object_t::test<4>()
     {
+        set_test_name("an expiry counts from now, not from the timer's last reset");
+        LLFrameTimer timer;
+        // Longer than the expiry passes between the timer starting and the
+        // expiry being set.
+        ms_sleep(300);
+        LLFrameTimer::updateFrameTime();
+        timer.setTimerExpirySec(0.2f);
+        ensure("not expired the moment it is set", !timer.hasExpired());
+        ensure("the whole expiry still to run", timer.getTimeToExpireF32() > 0.19f);
+    }
+
+    template<> template<>
+    void frametimer_object_t::test<5>()
+    {
+        set_test_name("a stopped timer keeps the time it ran, and resumes from it");
+        // Far enough from the start of the run that the absolute start time
+        // cannot pass for the time run.
+        ms_sleep(300);
+        LLFrameTimer::updateFrameTime();
+        LLFrameTimer timer;
+        timer.start();
+        ms_sleep(200);
+        LLFrameTimer::updateFrameTime();
+        const F32 ran = timer.getElapsedTimeF32();
+        timer.stop();
+        ensure("stopped", !timer.getStarted());
+        ensure_approximately_equals("the time it ran", timer.getElapsedTimeF32(), ran, 12);
+        ms_sleep(100);
+        LLFrameTimer::updateFrameTime();
+        ensure_approximately_equals("not running while stopped", timer.getElapsedTimeF32(), ran, 12);
+        timer.unpause();
+        ensure_approximately_equals("resumed from where it stopped", timer.getElapsedTimeF32(), ran, 12);
+    }
+
+    template<> template<>
+    void frametimer_object_t::test<6>()
+    {
+        set_test_name("a reset while paused zeroes the time run and leaves it paused");
+        ms_sleep(300);
+        LLFrameTimer::updateFrameTime();
+        LLFrameTimer timer;
+        timer.start();
+        ms_sleep(100);
+        LLFrameTimer::updateFrameTime();
+        timer.pause();
+        timer.reset();
+        ensure("still paused", !timer.getStarted());
+        ensure_approximately_equals("no time run", timer.getElapsedTimeF32(), 0.f, 12);
+        ms_sleep(100);
+        LLFrameTimer::updateFrameTime();
+        ensure_approximately_equals("not running while paused", timer.getElapsedTimeF32(), 0.f, 12);
+        timer.unpause();
+        ensure_approximately_equals("runs on from zero", timer.getElapsedTimeF32(), 0.f, 12);
+        timer.pause();
+        timer.start();
+        ensure("started", timer.getStarted());
+        ensure_approximately_equals("start() runs from zero, paused or not", timer.getElapsedTimeF32(), 0.f, 12);
     }
-*/
 }