From 24f3478c9886c111fc4b9454f61e4eb258bb3e12 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 23 Jul 2020 11:30:54 -0700 Subject: [PATCH] post() with const ref. Added "safety" of passing move-able reference is an illusion since no use_count()==1 test is done. Instead extra (shallow) copies were made for each subscriber. Instead. Pass const reference, redefine MonitorControlOp::post() to transfer ownership, and make only a single copy in SharedPV::post(). --- example/mailbox.cpp | 2 +- example/spam.cpp | 2 +- example/ticker.cpp | 2 +- src/pvxs/sharedpv.h | 2 +- src/pvxs/source.h | 17 ++++++++++------- src/servermon.cpp | 4 ++-- src/sharedpv.cpp | 11 ++++++++--- test/testmon.cpp | 10 +++++----- 8 files changed, 29 insertions(+), 21 deletions(-) diff --git a/example/mailbox.cpp b/example/mailbox.cpp index c8c5a8e..6f38eb1 100644 --- a/example/mailbox.cpp +++ b/example/mailbox.cpp @@ -74,7 +74,7 @@ int main(int argc, char* argv[]) // (optional) update the SharedPV cache and send // a update to any subscribers - pv.post(std::move(top)); + pv.post(top); // Required. Inform client that PUT operation is complete. op->reply(); diff --git a/example/spam.cpp b/example/spam.cpp index 0868e8b..7a483b8 100644 --- a/example/spam.cpp +++ b/example/spam.cpp @@ -62,7 +62,7 @@ public: log_debug_printf(app, "%s count %u\n", sub->peerName().c_str(), unsigned(cnt)); - }while(sub->tryPost(std::move(update))); + }while(sub->tryPost(update)); }; sub->onHighMark(fill); diff --git a/example/ticker.cpp b/example/ticker.cpp index d7cd663..3b8f40a 100644 --- a/example/ticker.cpp +++ b/example/ticker.cpp @@ -100,7 +100,7 @@ int main(int argc, char* argv[]) val["value"] = count++; - pv.post(std::move(val)); + pv.post(val); std::cout<<"Count "<0u @endcode - bool forcePost(Value&& val) { - return doPost(std::move(val), false, true); + //! @warning Caller must not modify the Value + bool forcePost(const Value& val) { + return doPost(val, false, true); } //! Add a new entry to the monitor queue. //! If nFree()<=0 this element will be "squashed" to the last element in the queue //! Returns @code nFree()>0u @endcode - bool post(Value&& val) { - return doPost(std::move(val), false, false); + //! @warning Caller must not modify the Value + bool post(const Value& val) { + return doPost(val, false, false); } //! Add a new entry to the monitor queue. //! If nFree()<=0 return false and take no other action //! Returns @code nFree()>0u @endcode - bool tryPost(Value&& val) { - return doPost(std::move(val), true, false); + //! @warning Caller must not modify the Value + bool tryPost(const Value& val) { + return doPost(val, true, false); } //! Signal to subscriber that this subscription will not yield any further events. diff --git a/src/servermon.cpp b/src/servermon.cpp index f541cc9..1d37030 100644 --- a/src/servermon.cpp +++ b/src/servermon.cpp @@ -209,7 +209,7 @@ struct ServerMonitorControl : public server::MonitorControlOp finish(); } - virtual bool doPost(Value&& val, bool maybe, bool force) override final + virtual bool doPost(const Value& val, bool maybe, bool force) override final { auto mon(op.lock()); if(!mon) @@ -221,7 +221,7 @@ struct ServerMonitorControl : public server::MonitorControlOp throw std::logic_error("Type change not allowed in post(). Recommend pvxs::Value::cloneEmpty()"); if((mon->queue.size() < mon->limit) || force || !val) { - mon->queue.push_back(std::move(val)); + mon->queue.push_back(val); } else if(!maybe) { // squash diff --git a/src/sharedpv.cpp b/src/sharedpv.cpp index 0f6c9ea..219b6a8 100644 --- a/src/sharedpv.cpp +++ b/src/sharedpv.cpp @@ -69,7 +69,7 @@ SharedPV SharedPV::buildMailbox() } } - pv.post(std::move(val)); + pv.post(val); op->reply(); }); @@ -381,7 +381,7 @@ void SharedPV::close() } } -void SharedPV::post(Value&& val) +void SharedPV::post(const Value& val) { if(!impl) throw std::logic_error("Empty SharedPV"); @@ -397,8 +397,13 @@ void SharedPV::post(Value&& val) impl->current.assign(val); + if(impl->subscribers.empty()) + return; + + auto copy(val.clone()); + for(auto& sub : impl->subscribers) { - sub->post(val.clone()); + sub->post(copy); } } diff --git a/test/testmon.cpp b/test/testmon.cpp index 07c5cde..788fc80 100644 --- a/test/testmon.cpp +++ b/test/testmon.cpp @@ -68,7 +68,7 @@ struct BasicTest { { auto update(initial.cloneEmpty()); update["value"] = v; - mbox.post(std::move(update)); + mbox.post(update); } static @@ -183,7 +183,7 @@ struct TestLifeCycle : public BasicTest auto update(initial.cloneEmpty()); update["value"] = 39; - mbox2.post(std::move(update)); + mbox2.post(update); if(auto val = pop(sub2, evt2)) { testEq(val["value"].as(), 39); @@ -224,9 +224,9 @@ struct TestReconn : public BasicTest testFalse(sub->pop())<<"No events after Disconnect"; - mbox.post(std::move(initial - .cloneEmpty() - .update("value", 15))); + mbox.post(initial + .cloneEmpty() + .update("value", 15)); errlogFlush(); testDiag("Starting server");