From ace1a82c12f1b7ce34119a8c5272e171b814ca39 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Wed, 20 Oct 2021 20:06:28 -0500 Subject: [PATCH 1/6] caProvider: Convert caChannelList into a tsDLList This should provide a significant performance boost when creating many thousands of CA channels. The only time it is necessary to traverse the list is in the CAChannelProvider's destructor; when individual channels are added or destroyed they can insert or remove themselves from the list without having to do a search. --- src/ca/caChannel.cpp | 14 +++++++++----- src/ca/caChannel.h | 2 ++ src/ca/caProvider.cpp | 37 +++++++++++-------------------------- src/ca/caProviderPvt.h | 6 ++++-- 4 files changed, 26 insertions(+), 33 deletions(-) diff --git a/src/ca/caChannel.cpp b/src/ca/caChannel.cpp index ed02812a..79795f47 100644 --- a/src/ca/caChannel.cpp +++ b/src/ca/caChannel.cpp @@ -132,7 +132,7 @@ void CAChannel::activate(short priority) channelCreated = true; CAChannelProviderPtr provider(channelProvider.lock()); if (provider) - provider->addChannel(shared_from_this()); + provider->addChannel(*this); EXCEPTION_GUARD(req->channelCreated(Status::Ok, shared_from_this())); } else { @@ -167,10 +167,14 @@ void CAChannel::disconnectChannel() /* Clear CA Channel */ Attach to(ca_context); int result = ca_clear_channel(channelID); - if (result == ECA_NORMAL) return; - string mess("CAChannel::disconnectChannel() "); - mess += ca_message(result); - cerr << mess << endl; + if (result != ECA_NORMAL) { + string mess("CAChannel::disconnectChannel() "); + mess += ca_message(result); + cerr << mess << endl; + } + CAChannelProviderPtr provider(channelProvider.lock()); + if (provider) + provider->delChannel(*this); } chid CAChannel::getChannelID() diff --git a/src/ca/caChannel.h b/src/ca/caChannel.h index cf3e296f..bfe13d32 100644 --- a/src/ca/caChannel.h +++ b/src/ca/caChannel.h @@ -18,6 +18,7 @@ #include #include #include +#include #include @@ -66,6 +67,7 @@ class CAChannelGetField : class CAChannel : public Channel, + public tsDLNode, public NotifierClient, public std::tr1::enable_shared_from_this { diff --git a/src/ca/caProvider.cpp b/src/ca/caProvider.cpp index 8cc15b39..cdb32a7f 100644 --- a/src/ca/caProvider.cpp +++ b/src/ca/caProvider.cpp @@ -31,22 +31,9 @@ CAChannelProvider::CAChannelProvider(const std::tr1::shared_ptr & CAChannelProvider::~CAChannelProvider() { - std::queue channelQ; - { - std::vector::iterator it; - epicsGuard G(channelListMutex); - for (it = caChannelList.begin(); it != caChannelList.end(); ++it) - { - CAChannelPtr caChannel(it->lock()); - if (caChannel) - channelQ.push(caChannel); - } - caChannelList.clear(); - } - while (!channelQ.empty()) - { - channelQ.front()->disconnectChannel(); - channelQ.pop(); + epicsGuard G(channelListMutex); + while (CAChannel *ch = caChannelList.first()) { + ch->disconnectChannel(); // Removes itself from the list } } @@ -106,18 +93,16 @@ Channel::shared_pointer CAChannelProvider::createChannel( return CAChannel::create(shared_from_this(), channelName, priority, channelRequester); } -void CAChannelProvider::addChannel(const CAChannelPtr &channel) +void CAChannelProvider::addChannel(CAChannel &channel) { - std::vector::iterator it; epicsGuard G(channelListMutex); - for (it = caChannelList.begin(); it != caChannelList.end(); ++it) - { - if (it->expired()) { - *it = channel; - return; - } - } - caChannelList.push_back(channel); + caChannelList.add(channel); +} + +void CAChannelProvider::delChannel(CAChannel &channel) +{ + epicsGuard G(channelListMutex); + caChannelList.remove(channel); } void CAChannelProvider::configure(epics::pvData::PVStructure::shared_pointer /*configuration*/) diff --git a/src/ca/caProviderPvt.h b/src/ca/caProviderPvt.h index 4b66e875..dceaae28 100644 --- a/src/ca/caProviderPvt.h +++ b/src/ca/caProviderPvt.h @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -80,7 +81,8 @@ class CAChannelProvider : virtual void flush(); virtual void poll(); - void addChannel(const CAChannelPtr & channel); + void addChannel(CAChannel &channel); + void delChannel(CAChannel &channel); CAContextPtr caContext() { return ca_context; @@ -94,7 +96,7 @@ class CAChannelProvider : private: CAContextPtr ca_context; epicsMutex channelListMutex; - std::vector caChannelList; + tsDLList caChannelList; NotifierConveyor connectNotifier; NotifierConveyor resultNotifier; From b8979b4152f4152daee43af423532ade006d46ea Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Wed, 20 Oct 2021 20:06:55 -0500 Subject: [PATCH 2/6] Reject empty channel names --- src/ca/caChannel.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ca/caChannel.cpp b/src/ca/caChannel.cpp index 79795f47..0ebe74bc 100644 --- a/src/ca/caChannel.cpp +++ b/src/ca/caChannel.cpp @@ -116,6 +116,8 @@ CAChannel::CAChannel(std::string const & channelName, connectNotification(new Notification()), ca_context(channelProvider->caContext()) { + if (channelName.empty()) + throw std::invalid_argument("Channel name cannot be empty"); } void CAChannel::activate(short priority) From 8cbd48faf31484b0c01e7b29c19266b7b79b265b Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Wed, 20 Oct 2021 20:07:50 -0500 Subject: [PATCH 3/6] Clean up CAChannel (done in disconnectChannel()) --- src/ca/caChannel.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/ca/caChannel.cpp b/src/ca/caChannel.cpp index 0ebe74bc..0f5072f4 100644 --- a/src/ca/caChannel.cpp +++ b/src/ca/caChannel.cpp @@ -145,10 +145,6 @@ void CAChannel::activate(short priority) CAChannel::~CAChannel() { - { - epicsGuard G(requestsMutex); - if (!channelCreated) return; - } disconnectChannel(); } From c44f90fd49fcd189302a6d630068913af4386919 Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Thu, 21 Oct 2021 13:07:14 -0500 Subject: [PATCH 4/6] CAChannel: Prevent problems during cleanup Guard channelCreated with requestsMutex. Remove channel from provider before clearing channelCreated. Simplify status handling. --- src/ca/caChannel.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/ca/caChannel.cpp b/src/ca/caChannel.cpp index 0f5072f4..2bbc1e97 100644 --- a/src/ca/caChannel.cpp +++ b/src/ca/caChannel.cpp @@ -130,17 +130,18 @@ void CAChannel::activate(short priority) ca_connection_handler, this, priority, // TODO mapping &channelID); + Status errorStatus; if (result == ECA_NORMAL) { - channelCreated = true; + epicsGuard G(requestsMutex); + channelCreated = true; // Set before addChannel() CAChannelProviderPtr provider(channelProvider.lock()); if (provider) provider->addChannel(*this); - EXCEPTION_GUARD(req->channelCreated(Status::Ok, shared_from_this())); } else { - Status errorStatus(Status::STATUSTYPE_ERROR, string(ca_message(result))); - EXCEPTION_GUARD(req->channelCreated(errorStatus, shared_from_this())); + errorStatus = Status::error(ca_message(result)); } + EXCEPTION_GUARD(req->channelCreated(errorStatus, shared_from_this())); } CAChannel::~CAChannel() @@ -153,7 +154,10 @@ void CAChannel::disconnectChannel() { epicsGuard G(requestsMutex); if (!channelCreated) return; - channelCreated = false; + CAChannelProviderPtr provider(channelProvider.lock()); + if (provider) + provider->delChannel(*this); + channelCreated = false; // Clear only after delChannel() } std::vector::iterator it; for (it = monitorlist.begin(); it!=monitorlist.end(); ++it) { @@ -170,9 +174,6 @@ void CAChannel::disconnectChannel() mess += ca_message(result); cerr << mess << endl; } - CAChannelProviderPtr provider(channelProvider.lock()); - if (provider) - provider->delChannel(*this); } chid CAChannel::getChannelID() From 3a20a064e57662415ac96faa26814f3065e7232a Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Thu, 21 Oct 2021 14:23:40 -0500 Subject: [PATCH 5/6] CAProvider: Fix cleanup during provider destruction CAChannel's channelProvider pointers have all expired by now, so the destructor must empty caChannelList itself. --- src/ca/caProvider.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ca/caProvider.cpp b/src/ca/caProvider.cpp index cdb32a7f..c4813d8f 100644 --- a/src/ca/caProvider.cpp +++ b/src/ca/caProvider.cpp @@ -32,8 +32,11 @@ CAChannelProvider::CAChannelProvider(const std::tr1::shared_ptr & CAChannelProvider::~CAChannelProvider() { epicsGuard G(channelListMutex); - while (CAChannel *ch = caChannelList.first()) { - ch->disconnectChannel(); // Removes itself from the list + while (CAChannel *ch = caChannelList.get()) { + // Here disconnectChannel() can't call our delChannel() + // beacuse its CAChannelProviderPtr has by now expired. + // That's why we removed it from caChannelList above. + ch->disconnectChannel(); } } From 5594d426563c8ebeccd8599ad31dd0425223ae7d Mon Sep 17 00:00:00 2001 From: Andrew Johnson Date: Thu, 28 Oct 2021 14:31:05 -0500 Subject: [PATCH 6/6] Release notes for use-tsDLList changes --- documentation/release_notes.dox | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/documentation/release_notes.dox b/documentation/release_notes.dox index 84768d0a..1c95adf8 100644 --- a/documentation/release_notes.dox +++ b/documentation/release_notes.dox @@ -1,5 +1,13 @@ /** @page pvarelease_notes Release Notes +Release 7.1.6 (UNRELEASED) +========================= + +- Changes to caProvider + - More internal changes to improve performance when connecting tens of + thousands of CA channels. + + Release 7.1.5 (October 2021) ==========================