From 5422c63fecdcb3cd164e339209e8b2c1b38ae79a Mon Sep 17 00:00:00 2001 From: Zhenyu Guo Date: Thu, 17 Dec 2015 15:01:31 +0800 Subject: [PATCH] fix timer bug in perf counters --- src/core/tests/perf_counter.cpp | 9 +----- src/core/tools/common/simple_perf_counter.cpp | 28 +++++++++++++------ .../common/simple_perf_counter_v2_atomic.cpp | 25 +++++++++++------ .../common/simple_perf_counter_v2_fast.cpp | 24 ++++++++++------ 4 files changed, 53 insertions(+), 33 deletions(-) diff --git a/src/core/tests/perf_counter.cpp b/src/core/tests/perf_counter.cpp index 65da2726a8..28904ef9e5 100644 --- a/src/core/tests/perf_counter.cpp +++ b/src/core/tests/perf_counter.cpp @@ -92,20 +92,15 @@ static void test_perf_counter() std::vector gen_numbers{1, 5, 1043}; int sleep_interval = config()->get_value("components.simple_perf_counter", "counter_computation_interval_seconds", 3, "period"); - perf_counter_impl* counter = new perf_counter_impl("", "", dsn_perf_counter_type_t::COUNTER_TYPE_NUMBER,""); + perf_counter_ptr counter = new perf_counter_impl("", "", dsn_perf_counter_type_t::COUNTER_TYPE_NUMBER, ""); perf_counter_inc_dec(counter); perf_counter_add(counter, vec); ddebug("%lf", counter->get_value()); - //don't delete the counter as it is shared by timer callback - //delete counter; - counter = new perf_counter_impl("", "", dsn_perf_counter_type_t::COUNTER_TYPE_RATE,""); perf_counter_inc_dec(counter); perf_counter_add(counter, vec); ddebug("%lf", counter->get_value()); - //don't delete the counter as it is shared by timer callback - //delete counter; counter = new perf_counter_impl("", "", dsn_perf_counter_type_t::COUNTER_TYPE_NUMBER_PERCENTILES,""); std::this_thread::sleep_for(std::chrono::seconds(sleep_interval)); @@ -116,8 +111,6 @@ static void test_perf_counter() for (int i=0; i!=COUNTER_PERCENTILE_COUNT; ++i) ddebug("%lf", counter->get_percentile((dsn_perf_counter_percentile_type_t)i)); } - //don't delete the counter as it is shared by timer callback - //delete counter; } TEST(tools_common, simple_perf_counter) diff --git a/src/core/tools/common/simple_perf_counter.cpp b/src/core/tools/common/simple_perf_counter.cpp index ce809dd9eb..170a98243b 100644 --- a/src/core/tools/common/simple_perf_counter.cpp +++ b/src/core/tools/common/simple_perf_counter.cpp @@ -120,14 +120,15 @@ namespace dsn { _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); _timer->expires_from_now(boost::posix_time::seconds(rand() % _counter_computation_interval_seconds + 1)); - _timer->async_wait(std::bind(&perf_counter_number_percentile::on_timer, this, std::placeholders::_1)); + + this->add_ref(); + _timer->async_wait(std::bind(&perf_counter_number_percentile::on_timer, this, _timer, std::placeholders::_1)); memset(_samples, 0, sizeof(_samples)); } ~perf_counter_number_percentile(void) { - _timer->cancel(); } virtual void increment() { dassert(false, "invalid execution flow"); } @@ -281,17 +282,22 @@ namespace dsn { return; } - void on_timer(const boost::system::error_code& ec) + void on_timer(std::shared_ptr timer, const boost::system::error_code& ec) { //as the callback is not in tls context, so the log system calls like ddebug, dassert will cause a lock if (!ec) { - boost::shared_ptr ctx(new compute_context()); - calc(ctx); + // only when others also hold the reference + if (this->get_count() > 1) + { + boost::shared_ptr ctx(new compute_context()); + calc(ctx); + + timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); - _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); - _timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); - _timer->async_wait(std::bind(&perf_counter_number_percentile::on_timer, this, std::placeholders::_1)); + this->add_ref(); + timer->async_wait(std::bind(&perf_counter_number_percentile::on_timer, this, timer, std::placeholders::_1)); + } } else if (boost::system::errc::operation_canceled == ec) { @@ -301,6 +307,8 @@ namespace dsn { { dassert(false, "on_timer error!!!"); } + + this->release_ref(); } std::shared_ptr _timer; @@ -321,11 +329,13 @@ namespace dsn { _counter_impl = new perf_counter_rate(section, name, type, dsptr); else _counter_impl = new perf_counter_number_percentile(section, name, type, dsptr); + + _counter_impl->add_ref(); } simple_perf_counter::~simple_perf_counter(void) { - delete _counter_impl; + _counter_impl->release_ref(); } } } diff --git a/src/core/tools/common/simple_perf_counter_v2_atomic.cpp b/src/core/tools/common/simple_perf_counter_v2_atomic.cpp index 60a97c9a46..56982270ad 100644 --- a/src/core/tools/common/simple_perf_counter_v2_atomic.cpp +++ b/src/core/tools/common/simple_perf_counter_v2_atomic.cpp @@ -184,7 +184,8 @@ namespace dsn { "period (seconds) the system computes the percentiles of the counters"); _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); _timer->expires_from_now(boost::posix_time::seconds(rand() % _counter_computation_interval_seconds + 1)); - _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_atomic::on_timer, this, std::placeholders::_1)); + this->add_ref(); + _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_atomic::on_timer, this, _timer, std::placeholders::_1)); } ~perf_counter_number_percentile_v2_atomic(void) @@ -327,22 +328,28 @@ namespace dsn { return; } - void on_timer(const boost::system::error_code& ec) + void on_timer(std::shared_ptr timer, const boost::system::error_code& ec) { //as the callback is not in tls context, so the log system calls like ddebug, dassert will cause a lock if (!ec) { - boost::shared_ptr ctx(new compute_context()); - calc(ctx); + // only when others also hold the reference + if (this->get_count() > 1) + { + boost::shared_ptr ctx(new compute_context()); + calc(ctx); - _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); - _timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); - _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_atomic::on_timer, this, std::placeholders::_1)); + timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); + this->add_ref(); + timer->async_wait(std::bind(&perf_counter_number_percentile_v2_atomic::on_timer, this, timer, std::placeholders::_1)); + } } else if (boost::system::errc::operation_canceled != ec) { dassert(false, "on_timer error!!!"); } + + this->release_ref(); } std::shared_ptr _timer; @@ -363,11 +370,13 @@ namespace dsn { _counter_impl = new perf_counter_rate_v2_atomic(section, name, type, dsptr); else _counter_impl = new perf_counter_number_percentile_v2_atomic(section, name, type, dsptr); + + _counter_impl->add_ref(); } simple_perf_counter_v2_atomic::~simple_perf_counter_v2_atomic(void) { - delete _counter_impl; + _counter_impl->release_ref(); } #pragma pack(pop) diff --git a/src/core/tools/common/simple_perf_counter_v2_fast.cpp b/src/core/tools/common/simple_perf_counter_v2_fast.cpp index 9f7ef2cc2f..25bf43556d 100644 --- a/src/core/tools/common/simple_perf_counter_v2_fast.cpp +++ b/src/core/tools/common/simple_perf_counter_v2_fast.cpp @@ -183,7 +183,8 @@ namespace dsn { "period (seconds) the system computes the percentiles of the counters"); _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); _timer->expires_from_now(boost::posix_time::seconds(rand() % _counter_computation_interval_seconds + 1)); - _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_fast::on_timer, this, std::placeholders::_1)); + this->add_ref(); + _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_fast::on_timer, this, _timer, std::placeholders::_1)); } ~perf_counter_number_percentile_v2_fast(void) @@ -326,22 +327,27 @@ namespace dsn { return; } - void on_timer(const boost::system::error_code& ec) + void on_timer(std::shared_ptr timer, const boost::system::error_code& ec) { //as the callback is not in tls context, so the log system calls like ddebug, dassert will cause a lock if (!ec) { - boost::shared_ptr ctx(new compute_context()); - calc(ctx); + // only when others also hold the reference + if (this->get_count() > 1) + { + boost::shared_ptr ctx(new compute_context()); + calc(ctx); - _timer.reset(new boost::asio::deadline_timer(shared_io_service::instance().ios)); - _timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); - _timer->async_wait(std::bind(&perf_counter_number_percentile_v2_fast::on_timer, this, std::placeholders::_1)); + timer->expires_from_now(boost::posix_time::seconds(_counter_computation_interval_seconds)); + this->add_ref(); + timer->async_wait(std::bind(&perf_counter_number_percentile_v2_fast::on_timer, this, timer, std::placeholders::_1)); + } } else if (boost::system::errc::operation_canceled != ec) { dassert(false, "on_timer error!!!"); } + this->release_ref(); } std::shared_ptr _timer; @@ -362,11 +368,13 @@ namespace dsn { _counter_impl = new perf_counter_rate_v2_fast(section, name, type, dsptr); else _counter_impl = new perf_counter_number_percentile_v2_fast(section, name, type, dsptr); + + _counter_impl->add_ref(); } simple_perf_counter_v2_fast::~simple_perf_counter_v2_fast(void) { - delete _counter_impl; + _counter_impl->release_ref(); } #pragma pack(pop)