[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636524987


   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;
   
   3. find something like a ConcurrentWeakHashMap, whitch is clean to use (both 
usage and license).
   
   4. create our own ConcurrentWeakHashMap class (not suggested but if give me 
a whole week I think I can do...)
   
   Which way should we enter?
   



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636474232


   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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636470709


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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636465013


   > If your app does not need synchronization, you can use the map in "fast" 
mode.
   
   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.
   ```
   @Override
   public V put(final K key, final V value) {
   if (fast) {
   synchronized (this) {
   final Map 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?



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636464477


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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636462475


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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636457266


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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636457321


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



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:
us...@infra.apache.org

[GitHub] [commons-beanutils] XenoAmess commented on pull request #27: [BEANUTILS-539] use ConcurrentHashMap insteadof WeakFastHashMap

2020-05-31 Thread GitBox 


XenoAmess commented on pull request #27:
URL: https://github.com/apache/commons-beanutils/pull/27#issuecomment-636433256


   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.
   



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:
us...@infra.apache.org