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 2022/08/04 06:51:49 UTC
[GitHub] [pulsar] coderzc opened a new pull request, #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
coderzc opened a new pull request, #16940:
URL: https://github.com/apache/pulsar/pull/16940
### Motivation
Now, Use a lot of locks to ensure the atomicity of `state_` in the `ConsumerImpl` and `ProducerImpl`, we can use atomic `state_` instead of the lock to improve performance.
### Modifications
Use an atomic `state_` instead of the lock to improve performance.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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] coderzc commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1207764771
/pulsarbot run-failure-checks
--
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] mattisonchao commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1207339337
@BewareMyPower @shibd Could you review it again?
--
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] shibd commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1205237650
@coderzc `PartitionedConsumerImpl` and `MultiTopicsConsumerImpl` may be also can improve
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r943352821
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -176,14 +172,14 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
// make sure we're still in the Pending/Ready state, closeAsync could have been invoked
// while waiting for this response if using lazy producers
- Lock lock(mutex_);
if (state_ != Ready && state_ != Pending) {
Review Comment:
```suggestion
const auto state = state_.load();
if (state != Ready && state != Pending) {
```
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r939773232
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.h:
##########
@@ -99,7 +99,7 @@ class MultiTopicsConsumerImpl : public ConsumerImplBase,
std::map<std::string, int> topicsPartitions_;
mutable std::mutex mutex_;
std::mutex pendingReceiveMutex_;
- MultiTopicsConsumerState state_ = Pending;
+ std::atomic<MultiTopicsConsumerState> state_ = {Pending};
Review Comment:
```suggestion
std::atomic<MultiTopicsConsumerState> state_{Pending};
```
`=` is not needed in brace initialization.
##########
pulsar-client-cpp/lib/PartitionedConsumerImpl.cc:
##########
@@ -389,20 +371,13 @@ void PartitionedConsumerImpl::notifyResult(CloseCallback closeCallback) {
}
}
-void PartitionedConsumerImpl::setState(const PartitionedConsumerState state) {
- Lock lock(mutex_);
- state_ = state;
- lock.unlock();
-}
+void PartitionedConsumerImpl::setState(const PartitionedConsumerState state) { state_ = state; }
Review Comment:
I think we don't need a `setState` method now with the atomic state. It's better to remove the `setState` method and use `state_ = xxx` instead.
--
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] shibd commented on a diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r939621227
##########
pulsar-client-cpp/lib/ConsumerImpl.cc:
##########
@@ -303,7 +299,6 @@ void ConsumerImpl::unsubscribeAsync(ResultCallback callback) {
void ConsumerImpl::handleUnsubscribe(Result result, ResultCallback callback) {
if (result == ResultOk) {
- Lock lock(mutex_);
state_ = Closed;
Review Comment:
Have a question:
What is the difference between direct assignment and `state_.store()`? Is there a visibility issue with the direct assignment?
--
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 merged pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16940:
URL: https://github.com/apache/pulsar/pull/16940
--
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 #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1211936169
Just left one final minor comment, LGTM.
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r943452351
##########
pulsar-client-cpp/lib/PatternMultiTopicsConsumerImpl.cc:
##########
@@ -55,8 +55,9 @@ void PatternMultiTopicsConsumerImpl::autoDiscoveryTimerTask(const boost::system:
return;
}
- if (state_ != Ready) {
- LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state_);
+ const auto state = state_.load();
+ if (state != Ready) {
+ LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state);
Review Comment:
I see. It makes sense.
--
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] coderzc commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1206018778
> @coderzc `PartitionedConsumerImpl` and `MultiTopicsConsumerImpl` may be also can improve
Thanks, I have already improved them.
--
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] coderzc commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1206018626
>
Thanks, I have already improved them.
--
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] coderzc commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1206289359
/pulsarbot run-failure-checks
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r937497183
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -726,10 +716,10 @@ Future<Result, ProducerImplBaseWeakPtr> ProducerImpl::getProducerCreatedFuture()
uint64_t ProducerImpl::getProducerId() const { return producerId_; }
void ProducerImpl::handleSendTimeout(const boost::system::error_code& err) {
- Lock lock(mutex_);
if (state_ != Pending && state_ != Ready) {
Review Comment:
```suggestion
const auto state = state_.load();
if (state != Pending && state != Ready) {
```
Not sure if the compiler will optimize it (read the same value for the two `state_ != xxx` comparisons). But it's better to get the current state only once explicitly.
--
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] coderzc commented on a diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r943450764
##########
pulsar-client-cpp/lib/PatternMultiTopicsConsumerImpl.cc:
##########
@@ -55,8 +55,9 @@ void PatternMultiTopicsConsumerImpl::autoDiscoveryTimerTask(const boost::system:
return;
}
- if (state_ != Ready) {
- LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state_);
+ const auto state = state_.load();
+ if (state != Ready) {
+ LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state);
Review Comment:
Oh, there is a log output `state` in the 60th line here, I want to ensure their consistency
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r943370561
##########
pulsar-client-cpp/lib/PartitionedConsumerImpl.cc:
##########
@@ -348,11 +329,12 @@ void PartitionedConsumerImpl::handleSinglePartitionConsumerClose(Result result,
}
}
void PartitionedConsumerImpl::closeAsync(ResultCallback callback) {
+ Lock lock(mutex_);
Review Comment:
We use `consumersMutex_` to protect `consumers_`.
##########
pulsar-client-cpp/lib/PartitionedConsumerImpl.cc:
##########
@@ -138,12 +126,10 @@ void PartitionedConsumerImpl::receiveAsync(ReceiveCallback& callback) {
void PartitionedConsumerImpl::unsubscribeAsync(ResultCallback callback) {
LOG_INFO("[" << topicName_->toString() << "," << subscriptionName_ << "] Unsubscribing");
// change state to Closing, so that no Ready state operation is permitted during unsubscribe
- setState(Closing);
+ state_ = Closing;
// do not accept un subscribe until we have subscribe to all of the partitions of a topic
// it's a logical single topic so it should behave like a single topic, even if it's sharded
- Lock lock(mutex_);
if (state_ != Ready) {
Review Comment:
We don't need the `state_ != Ready` check here anymore.
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r937497183
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -726,10 +716,10 @@ Future<Result, ProducerImplBaseWeakPtr> ProducerImpl::getProducerCreatedFuture()
uint64_t ProducerImpl::getProducerId() const { return producerId_; }
void ProducerImpl::handleSendTimeout(const boost::system::error_code& err) {
- Lock lock(mutex_);
if (state_ != Pending && state_ != Ready) {
Review Comment:
```suggestion
const auto state = state_;
if (state != Pending && state != Ready) {
```
Not sure if the compiler will optimize it (read the same value for the two `state_ != xxx` comparisons). But it's better to get the current state only once explicitly.
--
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] coderzc commented on a diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r939662300
##########
pulsar-client-cpp/lib/ConsumerImpl.cc:
##########
@@ -303,7 +299,6 @@ void ConsumerImpl::unsubscribeAsync(ResultCallback callback) {
void ConsumerImpl::handleUnsubscribe(Result result, ResultCallback callback) {
if (result == ResultOk) {
- Lock lock(mutex_);
state_ = Closed;
Review Comment:
> Have a question:
>
>
>
> What is the difference between direct assignment and `state_.store()`? Is there a visibility issue with the direct assignment?
>
>
The atomic variable overloaded the operator `=`, it is
equivalent to `store(desired)`.
More see：https://en.cppreference.com/w/cpp/atomic/atomic/operator%3D
--
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] coderzc commented on pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1208014005
@BewareMyPower PTAL
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r943435952
##########
pulsar-client-cpp/lib/PatternMultiTopicsConsumerImpl.cc:
##########
@@ -55,8 +55,9 @@ void PatternMultiTopicsConsumerImpl::autoDiscoveryTimerTask(const boost::system:
return;
}
- if (state_ != Ready) {
- LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state_);
+ const auto state = state_.load();
+ if (state != Ready) {
+ LOG_ERROR("Error in autoDiscoveryTimerTask consumer state not ready: " << state);
Review Comment:
This change is redundant. `if (state_ != Ready)` is equivalent to `if (state_.load() != Ready)`, see https://en.cppreference.com/w/cpp/atomic/atomic/operator_T.
Usually when `load()` is called once, we can simplify
```c++
const auto x = atomicValue.load();
if (x == expectedValue) { /* ... */ }
```
to
```c++
if (atomicValue == expectedValue) { /* ... */ }
```
##########
pulsar-client-cpp/lib/PartitionedConsumerImpl.cc:
##########
@@ -583,11 +550,11 @@ void PartitionedConsumerImpl::getPartitionMetadata() {
void PartitionedConsumerImpl::handleGetPartitions(Result result,
const LookupDataResultPtr& lookupDataResult) {
- Lock stateLock(mutex_);
if (state_ != Ready) {
return;
}
+ Lock stateLock(mutex_);
Review Comment:
```suggestion
```
`stateLock` is used to protect `state_` before. We don't need the lock anymore. Here it is because the previous code forgets to unlock.
--
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 #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#issuecomment-1211847327
The correct process on the following code might be a little complicated. I think we can add a TODO here.
```c++
if (state_ != Ready && state_ != Pending) {
state_ = Closed;
```
--
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 diff in pull request #16940: [improve][client-c++] Use an atomic `state_` instead of the lock to improve performance
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16940:
URL: https://github.com/apache/pulsar/pull/16940#discussion_r939774330
##########
pulsar-client-cpp/lib/PartitionedConsumerImpl.h:
##########
@@ -92,7 +92,7 @@ class PartitionedConsumerImpl : public ConsumerImplBase,
mutable std::mutex consumersMutex_;
mutable std::mutex mutex_;
std::mutex pendingReceiveMutex_;
- PartitionedConsumerState state_ = Pending;
+ std::atomic<PartitionedConsumerState> state_ = {Pending};
Review Comment:
```suggestion
std::atomic<PartitionedConsumerState> state_{Pending};
```
--
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