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 2020/03/26 01:41:54 UTC
[GitHub] [pulsar] yjshen opened a new pull request #6606: Fix non-atomic
volatile update
yjshen opened a new pull request #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604194207
/puslarbot 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen removed a comment on issue #6606: Fix non-atomic
volatile update
Posted by GitBox <gi...@apache.org>.
yjshen removed a comment on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604193526
/puslarbot 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604218576
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604193526
/puslarbot 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604194350
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604207357
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen removed a comment on issue #6606: Fix non-atomic
volatile update
Posted by GitBox <gi...@apache.org>.
yjshen removed a comment on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604194350
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
merlimat commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-611101184
I take back the argument on performance impact.. since this is only adding the static updaters. Though in most cases these updaters are not necessary because the variables are designed to be updated from a single thread or already within the context of a mutex.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-611111573
Yeah, this change is more about adding the static updaters. It doesn't change the existing behaviors. The change here is more about making the volatile updates atomic and ensure a consistent pattern across the code. Since it is always a good practice to follow across the whole pulsar code base. Did you see any other problems with this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen removed a comment on issue #6606: Fix non-atomic
volatile update
Posted by GitBox <gi...@apache.org>.
yjshen removed a comment on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604207357
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604294046
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
merlimat commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-611095545
What is the motivation for this change? There were different specific reasons for not using volatile in these components.
Volatile has a performance penalty and should only be used if strictly needed. Also, it's not a panacea that fixes all concurrency issue.
As it is, I think this commit should be reverted.
@yjshen @sijie
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604257580
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604314545
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen removed a comment on issue #6606: Fix non-atomic
volatile update
Posted by GitBox <gi...@apache.org>.
yjshen removed a comment on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604212395
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen commented on issue #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
yjshen commented on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604212395
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on a change in pull request #6606: Fix
non-atomic volatile update
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#discussion_r405703767
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1759,7 +1762,7 @@ public boolean isWritable() {
}
public void startSendOperation(Producer producer, int msgSize) {
- messagePublishBufferSize += msgSize;
Review comment:
All operations on a connection are happening on same thread
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie merged pull request #6606: Fix non-atomic volatile
update
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on a change in pull request #6606: Fix
non-atomic volatile update
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#discussion_r405703155
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/collections/GrowablePriorityLongPairQueue.java
##########
@@ -204,7 +207,7 @@ private LongPair removeAtWithoutLock(int index) {
LongPair item = new LongPair(data[index], data[index + 1]);
data[index] = EmptyItem;
data[index + 1] = EmptyItem;
- --this.size;
Review comment:
This is already protected by a write lock. No need to use updater. The volatile is only used to ensure readers are reading the last value of the size.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] yjshen removed a comment on issue #6606: Fix non-atomic
volatile update
Posted by GitBox <gi...@apache.org>.
yjshen removed a comment on issue #6606: Fix non-atomic volatile update
URL: https://github.com/apache/pulsar/pull/6606#issuecomment-604194207
/puslarbot 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services