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.

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

cacheMap property optimization #4344

Closed
XiaoWeiKIN opened this issue Nov 26, 2020 · 12 comments
Closed

cacheMap property optimization #4344

XiaoWeiKIN opened this issue Nov 26, 2020 · 12 comments

Comments

@XiaoWeiKIN
Copy link
Contributor

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

I wonder why cacheMap properties use the AtomicReference wrapper in ClientWorker, but cacheMap writes with lock. What's the point of using AtomicReference? I think ConcurrentHashMap could be used to replace it with a lock-free operation to improve concurrency.

image

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 26, 2020

Because this is not an operation, concurrenthashmap can only guarantee its own operation, and there is a reference replacement operation here

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

Because this is not an operation, concurrenthashmap can only guarantee its own operation, and there is a reference replacement operation here

What should replace the reference?

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

Because this is not an operation, concurrenthashmap can only guarantee its own operation, and there is a reference replacement operation here

What's the meaning of using a set method to replace a reference after synchronized so why not use CAS?

Loading

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 26, 2020

Maybe what I said is not clear enough. In many cases, not only do operations on the cachemap, it is a complete operation unit, and then why not use concurrent operations, because essentially the contents of the concurrenthashmap are also synchronized for locking

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

Maybe what I said is not clear enough. In many cases, not only do operations on the cachemap, it is a complete operation unit, and then why not use concurrent operations, because essentially the contents of the concurrenthashmap are also synchronized for locking

I think this code can be modified in the following way:

 CacheData cache = getCache(dataId, group, tenant);
        if (null != cache) {
            return cache;
        }
        String key = GroupKey.getKeyTenant(dataId, group, tenant);
        synchronized (cacheMap) {
            CacheData cacheFromMap = getCache(dataId, group, tenant);
            // multiple listeners on the same dataid+group and race condition,so
            // double check again
            // other listener thread beat me to set to cacheMap
            if (null != cacheFromMap) {
                cache = cacheFromMap;
                // reset so that server not hang this check
                cache.setInitializing(true);
            } else {
                cache = new CacheData(configFilterChainManager, agent.getName(), dataId, group, tenant);
                // fix issue # 1317
                if (enableRemoteSyncConfig) {
                    String[] ct = getServerConfig(dataId, group, tenant, 3000L);
                    cache.setContent(ct[0]);
                }
            }

            Map<String, CacheData> copy = new HashMap<String, CacheData>(this.cacheMap.get());
            copy.put(key, cache);
            cacheMap.set(copy);
        }

like:

  cacheData = map.computeIfAbsent(GroupKey.getKeyTenant(dataId, group, tenant),key->{
            cache = new CacheData(configFilterChainManager, agent.getName(), dataId, group, tenant);
            // fix issue # 1317
            if (enableRemoteSyncConfig) {
                String[] ct = getServerConfig(dataId, group, tenant, 3000L);
                cache.setContent(ct[0]);
            }
            return cache;
        });

        cache.setInitializing(true);

computeIfAbsent guarantees atomic units.

Loading

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 26, 2020

Your writing code looks good, but unfortunately now the client only supports the 1.6 jdk version

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

Your writing code looks good, but unfortunately now the client only supports the 1.6 jdk versio
That's really a shame

Loading

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 26, 2020

But in version 2.0 we will upgrade to version 1.8 using java

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 26, 2020

But in version 2.0 we will upgrade to version 1.8 using java
ok, it should be optimized using AtomicReference as well, so I'll give it a try

Loading

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 26, 2020

Welcome to contribute your code

Loading

@XiaoWeiKIN
Copy link
Contributor Author

@XiaoWeiKIN XiaoWeiKIN commented Nov 27, 2020

In JDK1.6, putIfAbsent from ConcurrentHashMap is thread safe, using synchronized on a Node to reduce the chance of lock collisions compared to cachemap. AtomicReference on the other hand, guarantees that it will not be initialized multiple times

 private final ConcurrentHashMap<String, AtomicReference<CacheData>> cacheMap =
        new ConcurrentHashMap<String, AtomicReference<CacheData>>();

 String key = GroupKey.getKeyTenant(dataId, group, tenant);
        AtomicReference<CacheData> cacheData = new AtomicReference<CacheData>();
        // multiple listeners on the same dataid+group and race condition
        AtomicReference<CacheData> lastCacheData = cacheMap.putIfAbsent(key, cacheData);
        if (lastCacheData == null) {
            CacheData cache = new CacheData(configFilterChainManager, agent.getName(), dataId, group, tenant);
            // strengthen the check
            if (cacheData.compareAndSet(null, cache)) {
                //fix issue # 1317
                if (enableRemoteSyncConfig) {
                    String[] ct = getServerConfig(dataId, group, tenant, 3000L);
                    cache.setContent(ct[0]);
                }
                lastCacheData = cacheData;
            }
            return lastCacheData.get();
        }

Loading

@paderlol
Copy link
Collaborator

@paderlol paderlol commented Nov 27, 2020

It's looks good,u can try it and test it performance in Nacos

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants