Re: svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
2010/10/16 Konstantin Kolinko :
> 2010/10/16 Tim Whittington :
Modified:
tomcat/trunk/java/javax/el/BeanELResolver.java
tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
Modified: tomcat/trunk/java/javax/el/BeanELResolver.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?rev=1022606&r1=1022605&r2=1022606&view=diff
>>>
>>> (...)
public V get(K key) {
V value = this.eden.get(key);
if (value == null) {
- value = this.longterm.get(key);
+ synchronized (longterm) {
+ value = this.longterm.get(key);
+ }
if (value != null) {
this.eden.put(key, value);
}
@@ -344,7 +346,9 @@ public class BeanELResolver extends ELRe
public void put(K key, V value) {
if (this.eden.size() >= this.size) {
- this.longterm.putAll(this.eden);
+ synchronized (longterm) {
+ this.longterm.putAll(this.eden);
+ }
this.eden.clear();
}
this.eden.put(key, value);
>>>
>>> I think that a ReadWriteLock will be more suitable here, because
>>> there will be a thousand of reads for a single write.
>>
For a note: I was wrong here. A ReadWriteLock cannot be used, because
a get() operation on a Map is generally not guaranteed to do not
change the state of the map.
The WeakHashMap modifies itself in a get(): it deletes stale values.
The other JRE implementations may be even different.
Best regards,
Konstantin Kolinko
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
2010/10/16 Tim Whittington :
>>> Modified:
>>> tomcat/trunk/java/javax/el/BeanELResolver.java
>>> tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
>>> tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
>>>
>>> Modified: tomcat/trunk/java/javax/el/BeanELResolver.java
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?rev=1022606&r1=1022605&r2=1022606&view=diff
>>>
>>
>> (...)
>>> public V get(K key) {
>>> V value = this.eden.get(key);
>>> if (value == null) {
>>> - value = this.longterm.get(key);
>>> + synchronized (longterm) {
>>> + value = this.longterm.get(key);
>>> + }
>>> if (value != null) {
>>> this.eden.put(key, value);
>>> }
>>> @@ -344,7 +346,9 @@ public class BeanELResolver extends ELRe
>>>
>>> public void put(K key, V value) {
>>> if (this.eden.size() >= this.size) {
>>> - this.longterm.putAll(this.eden);
>>> + synchronized (longterm) {
>>> + this.longterm.putAll(this.eden);
>>> + }
>>> this.eden.clear();
>>> }
>>> this.eden.put(key, value);
>>>
>>
>> I think that a ReadWriteLock will be more suitable here, because
>> there will be a thousand of reads for a single write.
>
> This won't be straight forward since the data structure is being
> modified in the get - you can't upgrade a ReentrantReadWriteLock from
> a read lock to a write lock, so it doesn't handle this situation.
>
I was talking about the lock around the longterm map.
For the longterm map the operation in get() is a read, and the one
in put() is a write.
(As for the whole structure, yes, get() updates it).
> A ReentrantLock might do a better job than a synchronized block if
> there's a real concern about the cost of syncs, but if not a rewrite
> of the cache structure to use a more conventional LRU ordering on put
> might be the best way.
>
>
>>> this.eden.clear();
>> Shouldn't the above line be inside the lock as well?
>
> It doesn't really matter - there's a race on the overflow logic
> anyway, so it'll still be called twice (and the underlying map is
> concurrent).
>
Best regards,
Konstantin Kolinko
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
> This won't be straight forward since the data structure is being > modified in the get - you can't upgrade a ReentrantReadWriteLock from > a read lock to a write lock, so it doesn't handle this situation. Correction: the same lock is required to protect get and put operations on the WeakHashMap (same problem results though, since that lock would be necessity be the write lock). - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
>> Modified:
>> tomcat/trunk/java/javax/el/BeanELResolver.java
>> tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
>> tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
>>
>> Modified: tomcat/trunk/java/javax/el/BeanELResolver.java
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?rev=1022606&r1=1022605&r2=1022606&view=diff
>>
>
> (...)
>> public V get(K key) {
>> V value = this.eden.get(key);
>> if (value == null) {
>> - value = this.longterm.get(key);
>> + synchronized (longterm) {
>> + value = this.longterm.get(key);
>> + }
>> if (value != null) {
>> this.eden.put(key, value);
>> }
>> @@ -344,7 +346,9 @@ public class BeanELResolver extends ELRe
>>
>> public void put(K key, V value) {
>> if (this.eden.size() >= this.size) {
>> - this.longterm.putAll(this.eden);
>> + synchronized (longterm) {
>> + this.longterm.putAll(this.eden);
>> + }
>> this.eden.clear();
>> }
>> this.eden.put(key, value);
>>
>
> I think that a ReadWriteLock will be more suitable here, because
> there will be a thousand of reads for a single write.
This won't be straight forward since the data structure is being
modified in the get - you can't upgrade a ReentrantReadWriteLock from
a read lock to a write lock, so it doesn't handle this situation.
A ReentrantLock might do a better job than a synchronized block if
there's a real concern about the cost of syncs, but if not a rewrite
of the cache structure to use a more conventional LRU ordering on put
might be the best way.
>> this.eden.clear();
> Shouldn't the above line be inside the lock as well?
It doesn't really matter - there's a race on the overflow logic
anyway, so it'll still be called twice (and the underlying map is
concurrent).
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
2010/10/14 :
> Author: markt
> Date: Thu Oct 14 16:36:20 2010
> New Revision: 1022606
>
> URL: http://svn.apache.org/viewvc?rev=1022606&view=rev
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50078
> Thread safety in EL caches. Patch provided by Takayoshi Kimura
>
> Modified:
> tomcat/trunk/java/javax/el/BeanELResolver.java
> tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
> tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
>
> Modified: tomcat/trunk/java/javax/el/BeanELResolver.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?rev=1022606&r1=1022605&r2=1022606&view=diff
>
(...)
> public V get(K key) {
> V value = this.eden.get(key);
> if (value == null) {
> - value = this.longterm.get(key);
> + synchronized (longterm) {
> + value = this.longterm.get(key);
> + }
> if (value != null) {
> this.eden.put(key, value);
> }
> @@ -344,7 +346,9 @@ public class BeanELResolver extends ELRe
>
> public void put(K key, V value) {
> if (this.eden.size() >= this.size) {
> - this.longterm.putAll(this.eden);
> + synchronized (longterm) {
> + this.longterm.putAll(this.eden);
> + }
> this.eden.clear();
> }
> this.eden.put(key, value);
>
I think that a ReadWriteLock will be more suitable here, because
there will be a thousand of reads for a single write.
> this.eden.clear();
Shouldn't the above line be inside the lock as well?
Best regards,
Konstantin Kolinko
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
svn commit: r1022606 - in /tomcat/trunk/java: javax/el/BeanELResolver.java org/apache/el/lang/ExpressionBuilder.java org/apache/el/util/ConcurrentCache.java
Author: markt
Date: Thu Oct 14 16:36:20 2010
New Revision: 1022606
URL: http://svn.apache.org/viewvc?rev=1022606&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50078
Thread safety in EL caches. Patch provided by Takayoshi Kimura
Modified:
tomcat/trunk/java/javax/el/BeanELResolver.java
tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
Modified: tomcat/trunk/java/javax/el/BeanELResolver.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/javax/el/BeanELResolver.java?rev=1022606&r1=1022605&r2=1022606&view=diff
==
--- tomcat/trunk/java/javax/el/BeanELResolver.java (original)
+++ tomcat/trunk/java/javax/el/BeanELResolver.java Thu Oct 14 16:36:20 2010
@@ -334,7 +334,9 @@ public class BeanELResolver extends ELRe
public V get(K key) {
V value = this.eden.get(key);
if (value == null) {
-value = this.longterm.get(key);
+synchronized (longterm) {
+value = this.longterm.get(key);
+}
if (value != null) {
this.eden.put(key, value);
}
@@ -344,7 +346,9 @@ public class BeanELResolver extends ELRe
public void put(K key, V value) {
if (this.eden.size() >= this.size) {
-this.longterm.putAll(this.eden);
+synchronized (longterm) {
+this.longterm.putAll(this.eden);
+}
this.eden.clear();
}
this.eden.put(key, value);
Modified: tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java?rev=1022606&r1=1022605&r2=1022606&view=diff
==
--- tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java (original)
+++ tomcat/trunk/java/org/apache/el/lang/ExpressionBuilder.java Thu Oct 14
16:36:20 2010
@@ -49,7 +49,8 @@ import org.apache.el.util.MessageFactory
*/
public final class ExpressionBuilder implements NodeVisitor {
-private static final ConcurrentCache cache = new
ConcurrentCache(5000);
+private static final ConcurrentCache cache =
+new ConcurrentCache(5000);
private FunctionMapper fnMapper;
Modified: tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java?rev=1022606&r1=1022605&r2=1022606&view=diff
==
--- tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java (original)
+++ tomcat/trunk/java/org/apache/el/util/ConcurrentCache.java Thu Oct 14
16:36:20 2010
@@ -37,7 +37,9 @@ public final class ConcurrentCache
public V get(K k) {
V v = this.eden.get(k);
if (v == null) {
-v = this.longterm.get(k);
+synchronized (longterm) {
+v = this.longterm.get(k);
+}
if (v != null) {
this.eden.put(k, v);
}
@@ -47,7 +49,9 @@ public final class ConcurrentCache
public void put(K k, V v) {
if (this.eden.size() >= size) {
-this.longterm.putAll(this.eden);
+synchronized (longterm) {
+this.longterm.putAll(this.eden);
+}
this.eden.clear();
}
this.eden.put(k, v);
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
