Merge pull request #11504 from vespa-engine/balder/hide-clock-implementation

Hide implementation to reduce FastOS_ visibility.

8 files changed, 101 insertions, 59 deletions

diff --git a/fastos/src/vespa/fastos/thread.cpp b/fastos/src/vespa/fastos/thread.cpp
index 3df8fa584a7..3e2f2674d97 100644
--- a/fastos/src/vespa/fastos/thread.cpp
+++ b/fastos/src/vespa/fastos/thread.cpp
@@ -352,17 +352,12 @@ void FastOS_ThreadInterface::Join ()
 // FastOS_Runnable
 // ----------------------------------------------------------------------
 
-FastOS_Runnable::FastOS_Runnable(void)
+FastOS_Runnable::FastOS_Runnable()
     : _thread(nullptr)
 {
 }
 
-FastOS_Runnable::~FastOS_Runnable(void)
+FastOS_Runnable::~FastOS_Runnable()
 {
     //      assert(_thread == nullptr);
 }
-
-void FastOS_Runnable::Detach(void)
-{
-    _thread = nullptr;
-}
diff --git a/fastos/src/vespa/fastos/thread.h b/fastos/src/vespa/fastos/thread.h
index c025a48d563..12866c71b2c 100644
--- a/fastos/src/vespa/fastos/thread.h
+++ b/fastos/src/vespa/fastos/thread.h
@@ -148,7 +148,7 @@ public:
     /**
      * Destructor. Closes pool if necessary.
      */
-    virtual ~FastOS_ThreadPool(void);
+    virtual ~FastOS_ThreadPool();
 
 
     /**
@@ -168,9 +168,9 @@ public:
      * Get the stack size used for threads in this pool.
      * @return          Stack size in bytes.
      */
-    int GetStackSize(void) const { return _stackSize; }
+    int GetStackSize() const { return _stackSize; }
 
-    int GetStackGuardSize(void) const { return 0; }
+    int GetStackGuardSize() const { return 0; }
 
     /**
      * Close the threadpool. This involves setting the break flag on
@@ -469,7 +469,7 @@ public:
  */
 class FastOS_Runnable
 {
-protected:
+private:
     friend class FastOS_ThreadInterface;
     FastOS_ThreadInterface *_thread;
 
@@ -498,10 +498,9 @@ public:
      */
     virtual void Run(FastOS_ThreadInterface *thisThread, void *arguments)=0;
 
-    FastOS_ThreadInterface *GetThread(void)             { return _thread; }
-    const FastOS_ThreadInterface *GetThread(void) const { return _thread; }
-    bool HasThread(void)                          const { return _thread != nullptr; }
-    void Detach(void);
+    FastOS_ThreadInterface *GetThread()             { return _thread; }
+    const FastOS_ThreadInterface *GetThread() const { return _thread; }
+    bool HasThread()                          const { return _thread != nullptr; }
 };
 
 #include <vespa/fastos/unix_thread.h>
diff --git a/searchcore/src/tests/proton/index/indexmanager_test.cpp b/searchcore/src/tests/proton/index/indexmanager_test.cpp
index 2f5d3e353db..51e12e70dda 100644
--- a/searchcore/src/tests/proton/index/indexmanager_test.cpp
+++ b/searchcore/src/tests/proton/index/indexmanager_test.cpp
@@ -1,7 +1,5 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
-#include <vespa/document/fieldvalue/document.h>
-#include <vespa/document/fieldvalue/fieldvalue.h>
 #include <vespa/fastos/file.h>
 #include <vespa/searchcore/proton/index/indexmanager.h>
 #include <vespa/searchcore/proton/server/executorthreadingservice.h>
@@ -19,12 +17,11 @@
 #include <vespa/searchlib/memoryindex/field_inverter.h>
 #include <vespa/searchlib/queryeval/isourceselector.h>
 #include <vespa/searchlib/test/index/mock_field_length_inspector.h>
-#include <vespa/searchlib/util/dirtraverse.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/io/fileutil.h>
-#include <vespa/vespalib/util/blockingthreadstackexecutor.h>
 #include <vespa/vespalib/util/threadstackexecutor.h>
 #include <set>
+#include <thread>
 
 #include <vespa/log/log.h>
 LOG_SETUP("indexmanager_test");
@@ -57,6 +54,7 @@ using vespalib::makeLambdaTask;
 using namespace proton;
 using namespace searchcorespi;
 using namespace searchcorespi::index;
+using namespace std::chrono_literals;
 
 namespace {
 
@@ -303,7 +301,7 @@ TEST_F(IndexManagerTest, require_that_memory_index_is_flushed)
         EXPECT_EQ(stat._modifiedTime, target.getLastFlushTime().timeSinceEpoch().time());
 
         // updated serial number & flush time when nothing to flush
-        FastOS_Thread::Sleep(8000);
+        std::this_thread::sleep_for(8s);
         fastos::TimeStamp now = fastos::ClockSystem::now().timeSinceEpoch();
         vespalib::Executor::Task::UP task;
         runAsMaster([&]() { task = target.initFlush(2); });
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index bbc599a661e..b85a1e00574 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -231,7 +231,7 @@ Proton::init()
 {
     assert( ! _initStarted && ! _initComplete );
     _initStarted = true;
-    if (_threadPool.NewThread(&_clock, nullptr) == nullptr) {
+    if (_threadPool.NewThread(_clock.getRunnable(), nullptr) == nullptr) {
         throw IllegalStateException("Failed starting thread for the cheap clock");
     }
     _protonConfigFetcher.start();
diff --git a/staging_vespalib/src/tests/clock/clock_benchmark.cpp b/staging_vespalib/src/tests/clock/clock_benchmark.cpp
index c5229adea31..a9618d50682 100644
--- a/staging_vespalib/src/tests/clock/clock_benchmark.cpp
+++ b/staging_vespalib/src/tests/clock/clock_benchmark.cpp
@@ -1,10 +1,13 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include <vespa/vespalib/util/clock.h>
+#include <vespa/fastos/thread.h>
 #include <cassert>
 #include <vector>
 #include <atomic>
 #include <cstring>
+#include <condition_variable>
+#include <mutex>
 
 using vespalib::Clock;
 using fastos::TimeStamp;
@@ -134,7 +137,7 @@ main(int , char *argv[])
     TestClock nsClock(nsValue, 1.0/frequency);
     TestClock nsVolatileClock(nsVolatile, 1.0/frequency);
     TestClock nsAtomicClock(nsAtomic, 1.0/frequency);
-    assert(pool.NewThread(&clock, nullptr) != nullptr);
+    assert(pool.NewThread(clock.getRunnable(), nullptr) != nullptr);
     assert(pool.NewThread(&nsClock, nullptr) != nullptr);
     assert(pool.NewThread(&nsVolatileClock, nullptr) != nullptr);
     assert(pool.NewThread(&nsAtomicClock, nullptr) != nullptr);
diff --git a/staging_vespalib/src/tests/clock/clock_test.cpp b/staging_vespalib/src/tests/clock/clock_test.cpp
index c8c93272e85..bf7e3773055 100644
--- a/staging_vespalib/src/tests/clock/clock_test.cpp
+++ b/staging_vespalib/src/tests/clock/clock_test.cpp
@@ -2,6 +2,7 @@
 
 #include <vespa/vespalib/testkit/testapp.h>
 #include <vespa/vespalib/util/clock.h>
+#include <vespa/fastos/thread.h>
 
 using vespalib::Clock;
 using fastos::TimeStamp;
@@ -20,7 +21,7 @@ Test::Main()
 
     Clock clock(0.050);
     FastOS_ThreadPool pool(0x10000);
-    ASSERT_TRUE(pool.NewThread(&clock, nullptr) != nullptr);
+    ASSERT_TRUE(pool.NewThread(clock.getRunnable(), nullptr) != nullptr);
     fastos::SteadyTimeStamp start = clock.getTimeNS();
     FastOS_Thread::Sleep(5000);
     fastos::SteadyTimeStamp stop = clock.getTimeNS();
diff --git a/staging_vespalib/src/vespa/vespalib/util/clock.cpp b/staging_vespalib/src/vespa/vespalib/util/clock.cpp
index e935a80bd6b..cd2a13029ab 100644
--- a/staging_vespalib/src/vespa/vespalib/util/clock.cpp
+++ b/staging_vespalib/src/vespa/vespalib/util/clock.cpp
@@ -1,17 +1,70 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "clock.h"
+#include <vespa/fastos/thread.h>
+#include <mutex>
+#include <condition_variable>
 #include <cassert>
 #include <chrono>
 
 namespace vespalib {
 
+namespace clock::internal {
+
+class Updater : public FastOS_Runnable
+{
+private:
+    Clock                   & _clock;
+    int                       _timePeriodMS;
+    std::mutex                _lock;
+    std::condition_variable   _cond;
+    bool                      _stop;
+
+
+    void Run(FastOS_ThreadInterface *thisThread, void *arguments) override;
+
+public:
+    Updater(Clock & clock, double timePeriod=0.100);
+    ~Updater();
+
+    void stop();
+};
+
+Updater::Updater(Clock & clock, double timePeriod)
+    : _clock(clock),
+      _timePeriodMS(static_cast<uint32_t>(timePeriod*1000)),
+      _lock(),
+      _cond(),
+      _stop(false)
+{ }
+
+Updater::~Updater() = default;
+
+void
+Updater::Run(FastOS_ThreadInterface *thread, void *)
+{
+    _clock._running = true;
+    std::unique_lock<std::mutex> guard(_lock);
+    while ( ! thread->GetBreakFlag() && !_stop) {
+        _clock.setTime();
+        _cond.wait_for(guard, std::chrono::milliseconds(_timePeriodMS));
+    }
+    _clock._running = false;
+}
+
+void
+Updater::stop()
+{
+    std::lock_guard<std::mutex> guard(_lock);
+    _stop = true;
+    _cond.notify_all();
+}
+
+}
+
 Clock::Clock(double timePeriod) :
      _timeNS(0u),
-     _timePeriodMS(static_cast<uint32_t>(timePeriod*1000)),
-     _lock(),
-     _cond(),
-     _stop(false),
+     _updater(std::make_unique<clock::internal::Updater>(*this, timePeriod)),
      _running(false)
 {
     setTime();
@@ -19,6 +72,9 @@ Clock::Clock(double timePeriod) :
 
 Clock::~Clock()
 {
+    if (_running) {
+        _updater->GetThread()->Join();
+    }
     assert(!_running);
 }
 
@@ -27,24 +83,15 @@ void Clock::setTime() const
     _timeNS.store(fastos::ClockSteady::now() - fastos::SteadyTimeStamp::ZERO, std::memory_order_relaxed);
 }
 
-void Clock::Run(FastOS_ThreadInterface *thread, void *arguments)
-{
-    (void) arguments;
-    _running = true;
-    std::unique_lock<std::mutex> guard(_lock);
-    while ( ! thread->GetBreakFlag() && !_stop) {
-        setTime();
-        _cond.wait_for(guard, std::chrono::milliseconds(_timePeriodMS));
-    }
-    _running = false;
-}
-
 void
 Clock::stop()
 {
-    std::lock_guard<std::mutex> guard(_lock);
-    _stop = true;
-    _cond.notify_all();
+    _updater->stop();
+}
+
+FastOS_Runnable *
+Clock::getRunnable() {
+    return _updater.get();
 }
 
 }
diff --git a/staging_vespalib/src/vespa/vespalib/util/clock.h b/staging_vespalib/src/vespa/vespalib/util/clock.h
index fc8e9eaff23..e9e1ebace3f 100644
--- a/staging_vespalib/src/vespa/vespalib/util/clock.h
+++ b/staging_vespalib/src/vespa/vespalib/util/clock.h
@@ -1,36 +1,32 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 #pragma once
 
-#include <vespa/fastos/thread.h>
 #include <vespa/fastos/timestamp.h>
-#include <mutex>
-#include <condition_variable>
+#include <atomic>
+#include <memory>
+
+class FastOS_Runnable;
 
 namespace vespalib {
 
+namespace clock::internal { class Updater; }
+
 /**
  * Clock is a clock that updates the time at defined intervals.
  * It is intended used where you want to check the time with low cost, but where
  * resolution is not that important.
  */
 
-class Clock : public FastOS_Runnable
+class Clock
 {
 private:
-    Clock(const Clock &);
-    Clock & operator = (const Clock &);
-
-    mutable std::atomic<int64_t>  _timeNS;
-    int                           _timePeriodMS;
-    std::mutex                    _lock;
-    std::condition_variable       _cond;
-    bool                          _stop;
-    bool                          _running;
+    mutable std::atomic<int64_t>              _timeNS;
+    std::unique_ptr<clock::internal::Updater> _updater;
+    std::atomic<bool>                         _running;
 
     void setTime() const;
-
-    void Run(FastOS_ThreadInterface *thisThread, void *arguments) override;
-
+    void start();
+    friend clock::internal::Updater;
 public:
     Clock(double timePeriod=0.100);
     ~Clock();
@@ -41,9 +37,12 @@ public:
         }
         return getTimeNSAssumeRunning();
     }
-    fastos::SteadyTimeStamp getTimeNSAssumeRunning() const { return fastos::SteadyTimeStamp(_timeNS.load(std::memory_order_relaxed)); }
+    fastos::SteadyTimeStamp getTimeNSAssumeRunning() const {
+        return fastos::SteadyTimeStamp(_timeNS.load(std::memory_order_relaxed));
+    }
 
     void stop();
+    FastOS_Runnable * getRunnable();
 };
 
 }