[BEANUTILS-539] use Concurrent(Weak)HashMap (or anything both faster and have better thread safety) insteadof WeakFastHashMap

[XenoAmess]

throughtout my performance test (using Jprofiler), I found out WeakHashMap is far slower than ConcurrentHashMap. add tests to show how slow it is. please run it using Jprofiler or other tools to see cpu call tree.

---

https://issues.apache.org/jira/browse/BEANUTILS-539

[XenoAmess]

feel free to ask for changes about this test, I will help to run it. Then we decide whether change usage of WeakHashMap to CuncurrentHashMap, or remain what it looks like for now.

[melloware]

Looks like you have some merge conflicts?

Also I think this will resolve BEANUTILS-509: https://github.com/apache/commons-beanutils/pull/21

[XenoAmess]

Looks like you have some merge conflicts?

Sorry for that. will fix it immediately.

Also I think this will resolve BEANUTILS-509: #21

I had no experience in using function Collections.synchronizedMap(), so I cannot tell about performance about that. Though I think this pr and that one are solving similiar things, though BEANUTILS-509 focus more on thread-safe, and this pr focus more on performance. And IMO if using ConcurrentHashMap, we can gain both thread-safe and performance.

[XenoAmess]

detailed output of Jprofile is attached at link : https://issues.apache.org/jira/browse/BEANUTILS-539

[garydgregory]

-1. Do you understand the difference between a weak and strong reference in a map? Do you you know what will happen if you install this change in an application that keeps putting new keys in the map and then not keep references to them?

[melloware]

@garydgregory I am having a hard time resyncing my fork all the sudden. I am getting this error "! refs/heads/master:refs/heads/master [remote rejected] (refusing to allow an OAuth App to create or update workflow .github/workflows/maven.yml without workflow scope) "

[XenoAmess]

-1. Do you understand the difference between a weak and strong reference in a map? Do you you know what will happen if you install this change in an application that keeps putting new keys in the map and then not keep references to them?

So weak reference is important here? Fine. Seems we need a ConcurrentWeakHashMap here. I will try to implement it, or at least do some refines on the current one. Willl close this pr and reopen when it finished.

[garydgregory]

If your app does not need synchronization, you can use the map in "fast" mode.

[XenoAmess]

@garydgregory is org.apache.commons.logging.impl.WeakHashtable a better replacement of WeakFastHashMap in this cases?

[XenoAmess]

If your app does not need synchronization, you can use the map in "fast" mode.

@garydgregory Thanks. usually we need synchronization, and even when I need weak reference I will never clone the whole map everytime putting a single key-value pair. I really doubt why we shall do it this way.

(from WeakFastHashMap.java)
@Override
    public V put(final K key, final V value) {
        if (fast) {
            synchronized (this) {
                final Map<K, V> temp = cloneMap(map);
                final V result = temp.put(key, value);
                map = temp;
                return result;
            }
        }
        synchronized (map) {
            return map.put(key, value);
        }
    }

Any suggestions? Should we make a counter and add 1 every time we put a key-value pair, and clone it only when the counter exceed some point?

[garydgregory]

IIRC I think someone contributed a concurrent weak hash map either in this component or in commons-collections, there should be a JIRA ticket... AFK ATM...

[XenoAmess]

IIRC I think someone contributed a concurrent weak hash map either in this component or in commons-collections, there should be a JIRA ticket... AFK ATM...

well org.apache.commons.logging.impl.WeakHashtable 's javadoc seems it be a good replacement of WeakFastHashMap here.

but it fails test MemoryLeakTestCase.testLocaleConvertUtilsBean_converters_memoryLeak. I will try tracking it.

----------

yes you are right, there be a jira ticket for a ConcurrentWeakHashMap, but nobody resolved it yet. https://issues.apache.org/jira/browse/COLLECTIONS-700

[XenoAmess]

also here is some guy contributed a concurrent weak hash map too : https://issues.apache.org/jira/browse/HARMONY-6434?jql=text%20~%20%22concurrent%20weak%20hash%20map%22

it also fails test MemoryLeakTestCase.testLocaleConvertUtilsBean_converters_memoryLeak.

[XenoAmess]

my bad. I found the reason why it can't pass, it is my mistake. Now we can use org.apache.commons.logging.impl.WeakHashtable to replace FastWeakHashMap. We can also use the ConcurrentWeakHashMap in https://issues.apache.org/jira/browse/HARMONY-6434 , but that class is from a strange place.

It said it be from package org.amino.ds.tmpMap;, but I download amino from sourceforge at https://sourceforge.net/projects/amino-cbbs/files/cbbs/1.0/amino-java-src-1.0.tar.gz/download, and 1.0 is the greatest version number there, and found no class like this contained.

Also, that lib is at apache license v2, so if we want to use it we must follow that license. Thus WeakHashtable might be the best choice here.

Though I still think a ConcurrentWeakHashMap should have better performance... but WeakHashtable is far better than WeakFastHashMap.

Only one more thing: WeakFastHashMap allows null key and null value(same as WeakHashMap do), but WeakHashtable does not allow null key nor null value(same as Hashtable do).

So what should we do next?

Actually there be several ways.

1. use WeakHashtable for now, and accept the BC that it cannot have null key/value;

2. wrap another layer for WeakHashtable, and make it support null key and null value that way;

3. fork WeakHashtable, and change its source codes to make it support null key and null value that way;

4. find something like a ConcurrentWeakHashMap, whitch is clean to use (both usage and license).

5. create our own ConcurrentWeakHashMap class (not suggested but if give me a whole week I think I can do...)

Which way should we enter?

[garydgregory]

Just wait. I have a query on the legal mailing list to check to see if we can reuse the Infinispan version of its concurrent weak key hash map, and since it is co-authored by Doug Lea it likely one of the better if not the best implementation out there.

[XenoAmess]

Just wait. I have a query on the legal mailing list to check to see if we can reuse the Infinispan version of its concurrent weak key hash map, and since it is co-authored by Doug Lea it likely one of the better if not the best implementation out there.

@garydgregory Hi gary. How is that concurrent weak key hash map going? Or any other news about this pr?

[melloware]

@garydgregory any update from the lawyers?

[melloware]

@garydgregory Just pinging you about this one again checking in?

[garydgregory]

No, but let me poke around...

[melloware]

Looks like Inifinispan is Apache 2.0 licensed if that matters and it looks like they no longer even use this class? Looks like it was removed in Infinispan 10.0

https://github.com/infinispan/infinispan/blob/0cbb18f426c803a3b769047da10d520b8e8b5fea/commons/all/src/main/java/org/infinispan/commons/util/concurrent/ConcurrentWeakKeyHashMap.java

[melloware]

See: https://github.com/apache/commons-beanutils/pull/37

[melloware]

@XenoAmess I think you are safe to close this PR as its subsumed by #37

[XenoAmess]

@XenoAmess I think you are safe to close this PR as its subsumed by #37

@melloware Well, I'd rather close this pr AFTER #37 is merged.

[melloware]

No problem.