You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/02 02:57:58 UTC
[GitHub] [pulsar] AndrewJD79 opened a new pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
AndrewJD79 opened a new pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079
Fixes #12844
### Motivation
1. ProducerStatsImpl (and ConsumerStatsImpl) classes use a single shared lock to protect access to internal data. This cause signficant contention on high load.
2. Data races between timer and object destructor
### Modifications
1. Use separate lock for updating send and receive statistics.
2. Add shared lock to prevent data races.
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: ( no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no )
### Documentation
- [x ] `no-need-doc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AndrewJD79 commented on a change in pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
AndrewJD79 commented on a change in pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#discussion_r761543260
##########
File path: pulsar-client-cpp/lib/stats/ConsumerStatsImpl.cc
##########
@@ -21,57 +21,84 @@
#include <lib/LogUtils.h>
#include <functional>
+#include <sstream>
namespace pulsar {
DECLARE_LOG_OBJECT();
+struct ConsumerStatsImpl::AsyncCallbackLock
+{
+ std::mutex mutex;
+ bool enabled = true;
Review comment:
Access to `enabled ` is always performed under mutex lock. Therefore atomic is not required here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#issuecomment-1072917185
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#issuecomment-1015480430
ping @merlimat Please help review this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AndrewJD79 commented on a change in pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
AndrewJD79 commented on a change in pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#discussion_r761542835
##########
File path: pulsar-client-cpp/lib/stats/ConsumerStatsImpl.h
##########
@@ -30,30 +30,37 @@ class ConsumerStatsImpl : public ConsumerStatsBase {
private:
std::string consumerStr_;
- unsigned long numBytesRecieved_ = 0;
- std::map<Result, unsigned long> receivedMsgMap_;
- std::map<std::pair<Result, proto::CommandAck_AckType>, unsigned long> ackedMsgMap_;
+ ExecutorServicePtr executor_;
+ DeadlineTimerPtr timer_;
+ unsigned int statsIntervalInSeconds_;
+
+ struct AsyncCallbackLock;
+ std::shared_ptr<AsyncCallbackLock> callbackLock_;
+ std::mutex rcvMutex_;
+ unsigned long numBytesRecieved_ = 0;
unsigned long totalNumBytesRecieved_ = 0;
Review comment:
Renamed to numBytesReceived_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AndrewJD79 commented on a change in pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
AndrewJD79 commented on a change in pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#discussion_r761543343
##########
File path: pulsar-client-cpp/lib/stats/ProducerStatsImpl.cc
##########
@@ -41,52 +42,60 @@ std::string ProducerStatsImpl::latencyToString(const LatencyAccumulator& obj) {
return os.str();
}
+struct ProducerStatsImpl::AsyncCallbackLock
+{
+ std::mutex mutex;
+ bool enabled = true;
Review comment:
Moved to AsyncCallbackLock.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#discussion_r760740364
##########
File path: pulsar-client-cpp/lib/stats/ProducerStatsImpl.cc
##########
@@ -41,52 +42,60 @@ std::string ProducerStatsImpl::latencyToString(const LatencyAccumulator& obj) {
return os.str();
}
+struct ProducerStatsImpl::AsyncCallbackLock
+{
+ std::mutex mutex;
+ bool enabled = true;
Review comment:
The similar question as before. BTW, I think we can wrap it in another header file. It only requires including `<mutex>` header so it's not a big issue.
##########
File path: pulsar-client-cpp/lib/stats/ConsumerStatsImpl.h
##########
@@ -30,30 +30,37 @@ class ConsumerStatsImpl : public ConsumerStatsBase {
private:
std::string consumerStr_;
- unsigned long numBytesRecieved_ = 0;
- std::map<Result, unsigned long> receivedMsgMap_;
- std::map<std::pair<Result, proto::CommandAck_AckType>, unsigned long> ackedMsgMap_;
+ ExecutorServicePtr executor_;
+ DeadlineTimerPtr timer_;
+ unsigned int statsIntervalInSeconds_;
+
+ struct AsyncCallbackLock;
+ std::shared_ptr<AsyncCallbackLock> callbackLock_;
+ std::mutex rcvMutex_;
+ unsigned long numBytesRecieved_ = 0;
unsigned long totalNumBytesRecieved_ = 0;
Review comment:
I think we can correct the original typo as well? `numBytesRecieved_` -> `numBytesReceived_`.
##########
File path: pulsar-client-cpp/lib/stats/ConsumerStatsImpl.cc
##########
@@ -21,57 +21,84 @@
#include <lib/LogUtils.h>
#include <functional>
+#include <sstream>
namespace pulsar {
DECLARE_LOG_OBJECT();
+struct ConsumerStatsImpl::AsyncCallbackLock
+{
+ std::mutex mutex;
+ bool enabled = true;
Review comment:
Should it be an `atomic_bool`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#issuecomment-994469106
@merlimat Please help review this PR, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #13079: [Issue 12844][pulsar-client-cpp] Excessive locking cause significant performance degradation
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #13079:
URL: https://github.com/apache/pulsar/pull/13079#issuecomment-984260796
Please use clang-format-5.0 to reformat your code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org