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

XenoAmess · 2020-05-31T07:15:36Z

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

please run it using Jprofiler or other tools to see cpu call tree.

XenoAmess · 2020-05-31T07:17:31Z

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 · 2020-05-31T11:09:17Z

Looks like you have some merge conflicts?

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

XenoAmess · 2020-05-31T11:18:55Z

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 · 2020-05-31T11:19:32Z

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

garydgregory · 2020-05-31T11:20:57Z

-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?

…into change_WeakFastHashMap_to_ConcurrentHashMap � Conflicts: � src/main/java/org/apache/commons/beanutils2/BeanUtils.java � src/main/java/org/apache/commons/beanutils2/ConvertUtilsBean.java

melloware · 2020-05-31T11:27:47Z

@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 · 2020-05-31T12:08:51Z

-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 · 2020-05-31T12:22:08Z

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

XenoAmess · 2020-05-31T12:26:21Z

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

XenoAmess · 2020-05-31T12:31:10Z

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 · 2020-05-31T12:40:06Z

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 · 2020-05-31T13:21:03Z

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 · 2020-05-31T13:50:58Z

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.

This reverts commit b26d0f4

XenoAmess · 2020-05-31T20:28:23Z

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.

use WeakHashtable for now, and accept the BC that it cannot have null key/value;
wrap another layer for WeakHashtable, and make it support null key and null value that way;
fork WeakHashtable, and change its source codes to make it support null key and null value that way;
find something like a ConcurrentWeakHashMap, whitch is clean to use (both usage and license).
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 · 2020-05-31T21:14:36Z

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 · 2020-06-14T13:38:28Z

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 · 2020-07-07T17:46:39Z

@garydgregory any update from the lawyers?

melloware · 2020-07-27T13:29:21Z

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

garydgregory · 2020-07-28T15:20:18Z

No, but let me poke around...

melloware · 2020-08-05T19:36:32Z

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 · 2020-08-28T17:36:40Z

See: #37

melloware · 2020-08-29T13:59:20Z

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

XenoAmess · 2020-12-15T15:22:26Z

@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 · 2020-12-15T15:33:00Z

No problem.

add tests to show how slow it is.

b6c8e49

please run it using Jprofiler or other tools to see cpu call tree.

try remove the use of WeakFastHashMap.

2c3e108

Merge branch 'master' of https://github.com/apache/commons-beanutils …

5c7e1cf

…into change_WeakFastHashMap_to_ConcurrentHashMap � Conflicts: � src/main/java/org/apache/commons/beanutils2/BeanUtils.java � src/main/java/org/apache/commons/beanutils2/ConvertUtilsBean.java

XenoAmess closed this May 31, 2020

change to org.apache.commons.logging.impl.WeakHashtable

54711ac

XenoAmess reopened this May 31, 2020

XenoAmess added 5 commits May 31, 2020 22:05

change to some other ConcurrentWeakHashMap

807209e

fix

f9f8773

trytofix

b26d0f4

Revert "trytofix"

b1f3e05

This reverts commit b26d0f4

change from to WeakFastHashMap to WeakHashtable

8b57699

XenoAmess changed the title ~~[BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap~~ [BEANUTILS-539] use Concurrent(Weak)HashMap (or anything both faster and have better thread safety) insteadof WeakFastHashMap Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

XenoAmess commented May 31, 2020 •

edited by garydgregory

XenoAmess commented May 31, 2020

melloware commented May 31, 2020

XenoAmess commented May 31, 2020

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

melloware commented May 31, 2020

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

XenoAmess commented May 31, 2020

XenoAmess commented May 31, 2020 •

edited

garydgregory commented May 31, 2020

XenoAmess commented May 31, 2020 •

edited

XenoAmess commented May 31, 2020 •

edited

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

XenoAmess commented Jun 14, 2020

melloware commented Jul 7, 2020

melloware commented Jul 27, 2020

garydgregory commented Jul 28, 2020

melloware commented Aug 5, 2020

melloware commented Aug 28, 2020

melloware commented Aug 29, 2020

XenoAmess commented Dec 15, 2020 •

edited

melloware commented Dec 15, 2020

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

Are you sure you want to change the base?

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

Conversation

XenoAmess commented May 31, 2020 • edited by garydgregory

XenoAmess commented May 31, 2020

melloware commented May 31, 2020

XenoAmess commented May 31, 2020

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

melloware commented May 31, 2020

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

XenoAmess commented May 31, 2020

XenoAmess commented May 31, 2020 • edited

garydgregory commented May 31, 2020

XenoAmess commented May 31, 2020 • edited

XenoAmess commented May 31, 2020 • edited

XenoAmess commented May 31, 2020

garydgregory commented May 31, 2020

XenoAmess commented Jun 14, 2020

melloware commented Jul 7, 2020

melloware commented Jul 27, 2020

garydgregory commented Jul 28, 2020

melloware commented Aug 5, 2020

melloware commented Aug 28, 2020

melloware commented Aug 29, 2020

XenoAmess commented Dec 15, 2020 • edited

melloware commented Dec 15, 2020

XenoAmess commented May 31, 2020 •

edited by garydgregory

XenoAmess commented May 31, 2020 •

edited

XenoAmess commented May 31, 2020 •

edited

XenoAmess commented May 31, 2020 •

edited

XenoAmess commented Dec 15, 2020 •

edited