Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve NacosNamingServiceWrapper performance #11665

Merged
merged 2 commits into from
Mar 8, 2023
Merged

improve NacosNamingServiceWrapper performance #11665

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f2f965
improve NacosNamingServiceWrapper performance
sourcelliu Feb 26, 2023
a1bec87
Merge branch '3.2' into improve
AlbumenJ Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 7 additions & 37 deletions ...gistry-nacos/src/main/java/org/apache/dubbo/registry/nacos/NacosNamingServiceWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ public class NacosNamingServiceWrapper {

private final ConcurrentMap<InstanceId, InstancesInfo> registerStatus = new ConcurrentHashMap<>();
private final ConcurrentMap<SubscribeInfo, NamingService> subscribeStatus = new ConcurrentHashMap<>();
private final Lock mapLock = new ReentrantLock();

public NacosNamingServiceWrapper(NacosConnectionManager nacosConnectionManager, int retryTimes, int sleepMsBetweenRetries) {
this.nacosConnectionManager = nacosConnectionManager;
Expand Down Expand Up @@ -108,13 +107,7 @@ public List<Instance> getAllInstances(String serviceName, String group) throws N

public void registerInstance(String serviceName, String group, Instance instance) throws NacosException {
String nacosServiceName = handleInnerSymbol(serviceName);
InstancesInfo instancesInfo;
try {
mapLock.lock();
instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());
} finally {
mapLock.unlock();
}
InstancesInfo instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());

try {
instancesInfo.lock();
Expand Down Expand Up @@ -176,13 +169,7 @@ public void registerInstance(String serviceName, String group, Instance instance

public void updateInstance(String serviceName, String group, Instance oldInstance, Instance newInstance) throws NacosException {
String nacosServiceName = handleInnerSymbol(serviceName);
InstancesInfo instancesInfo;
try {
mapLock.lock();
instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());
} finally {
mapLock.unlock();
}
InstancesInfo instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());

try {
instancesInfo.lock();
Expand Down Expand Up @@ -222,13 +209,7 @@ public void updateInstance(String serviceName, String group, Instance oldInstanc

public void deregisterInstance(String serviceName, String group, String ip, int port) throws NacosException {
String nacosServiceName = handleInnerSymbol(serviceName);
InstancesInfo instancesInfo;
try {
mapLock.lock();
instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());
} finally {
mapLock.unlock();
}
InstancesInfo instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());

try {
instancesInfo.lock();
Expand All @@ -249,13 +230,7 @@ public void deregisterInstance(String serviceName, String group, String ip, int

public void deregisterInstance(String serviceName, String group, Instance instance) throws NacosException {
String nacosServiceName = handleInnerSymbol(serviceName);
InstancesInfo instancesInfo;
try {
mapLock.lock();
instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());
} finally {
mapLock.unlock();
}
InstancesInfo instancesInfo = ConcurrentHashMapUtils.computeIfAbsent(registerStatus, new InstanceId(nacosServiceName, group), id -> new InstancesInfo());

try {
instancesInfo.lock();
Expand All @@ -269,14 +244,9 @@ public void deregisterInstance(String serviceName, String group, Instance instan
InstanceInfo instanceInfo = optional.get();
instancesInfo.getInstances().remove(instanceInfo);

try {
mapLock.lock();
if (instancesInfo.getInstances().isEmpty()) {
registerStatus.remove(new InstanceId(nacosServiceName, group));
instancesInfo.setValid(false);
}
} finally {
mapLock.unlock();
if (instancesInfo.getInstances().isEmpty()) {
registerStatus.remove(new InstanceId(nacosServiceName, group));
instancesInfo.setValid(false);
Comment on lines +247 to +249
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operation here is not atomic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlbumenJ
I think there is no need for atomicity guarantee here.

  • because the code here has already got instancesInfo lock instancesInfo.lock();
  • and for the modification of registerStatus registerStatus.remove(new InstanceId(nacosServiceName, group));, due to the protection of the lock mechanism inside concurrenthashmap , which has the same effect as the previous mapLock.lock()

}

// only one registered
Expand Down