Lock Contention on org.jboss.ejb.BeanLockManager

[clebert.suconic]

As for http://jira.jboss.com/jira/browse/JBAS-2428 the contention happens when we have a huge number of threads accessing a single EJB (on my testcase a Stateful Session Bean).

We are doing this tests in a huge hardware, and that's why we are being able to hit more than 800 threads in a single box. But for scaling more in a single CPU we need to solve that contention.

I have tried to change the code a little bit, but we are seeing better responses but the contention still exists:

 - private Map map = Collections.synchronizedMap(new HashMap());
 + private HashMap map = new HashMap();




 public BeanLock getLock(Object id)
 {
 if (id == null)
 throw new IllegalArgumentException("Attempt to get lock ref with a null object");
 BeanLock lock = (BeanLock) map.get(id);


 if (lock == null)
 {
 // to guarantee atomicity we will redo the map.get
 synchronized (this)
 {
 lock = (BeanLock)map.get(id);
 if (lock==null)
 {
 try
 {
 lock = (BeanLock) lockClass.newInstance();
 lock.setId(id);
 lock.setTimeout(txTimeout);
 lock.setContainer(container);
 if( trace )
 log.trace("Created lock: "+lock);
 }
 catch (Exception e)
 {
 // schrouf: should we really proceed with lock object
 // in case of exception ??
 log.warn("Failed to initialize lock:"+lock, e);
 }
 map.put(id, lock);
 }
 }

 }
 synchronized (lock)
 {
 lock.addRef();
 }
 if( trace )
 log.trace("Added ref to lock: "+lock);

 return lock;
 }


>> on removeLockREf:

 synchronized (lock)
 {
 lock.removeRef();
 }



>> on canPassivate:

 synchronized (lock)
 {
 return (lock.getRefs() <= 1);
 }

An option would be to have more than one BeanLockManager per container (using some sort of parameter, like scallingFactor) and divide the data

[clebert.suconic]

Elias Ross suggested this fix:

One thing you must *not* ever do is call HashMap.get() without a lock. It can cause an infinite loop (hang) in your code if it is being changed in a different thread!

The contention is undoubtedly taking place in the creation stage.
I think this might improve things:

public BeanLock getLock(Object id)
{
synchronized (this) {
BeanLock lock = (BeanLock) map.get(id);
if (id != null) return lock;
}
BeanLock lock2 = .... //// Create a new lock
synchronized (this) {
BeanLock lock = (BeanLock) map.get(id);
if (id != null) return lock;
// Oops, have to toss away a perfectly good lock, but oh well.
map.put(id, lock2);
return lock2;
}
}

But I think it won't work as Adrian said in the developer's mail list, as this need to be atomic.

[adrian.brock]

Elias Ross suggested this fix:

But I think it won't work as Adrian said in the developer's mail list, as this need to be atomic.

It will work. That is the form of the double check that does work! :-)

The key constraint is that you shouldn't have two different lock objects for the same
session id.
You code ensures there is only one lock object, so it will work as per contract.

What we really want is a concurrent map implementation that lets you

Object previous = map.putIfNotPresent(key, new);

i.e. if there is a previous value for the key, it gives you it atomically
disregards your put. The code then becomes:

public BeanLock getLock(Object id)
{
BeanLock lock = (BeanLock) map.get(id);
if (lock != null) return lock;
BeanLock lock2 = .... //// Create a new lock
return (BeanLock) map.putIfNotPresent(id, lock2); // lock2 is discarded if already present.
}

[adrian.brock]

It will work. That is the form of the double check that does work! :-)

Or at least it work here because the lock creation does not compete with map removal.

[clebert.suconic]

I am trying Elias' changes for this case.

I'm uploading the modified BeanLockManager.java into JIRA, and I will be testing it.


I have already tested with low load and it is working. I will now test it with higher Injection Rate and see if the contention is gone.

[clebert.suconic]

We did some tests with this change.

Number of locks have diminished but we're still seeing lock contention.


I have another change in top of that (keeping Elias' suggestion):

I will be creating 10 instances of HashMap, and switch between instances using id.hashMap()%10.

This is working already and I will be testing it anyway. I'm uploading the latest change to the case.


BTW: It would be nice if I could have a way to parametrize the number of instances. I could use System.property if there isn't any other possibilities.

[adrian.brock]

I have another change in top of that (keeping Elias' suggestion):

I will be creating 10 instances of HashMap, and switch between instances using id.hashMap()%10.

That is ConcurrentHashMap from oswego concurrent, use that instead!
It uses 40 partitions IIRC.

[adrian.brock]

I have another change in top of that (keeping Elias' suggestion):

I will be creating 10 instances of HashMap, and switch between instances using id.hashMap()%10.

That is ConcurrentHashMap from oswego concurrent, use that instead!
It uses 40 partitions IIRC.

Although Oswego's version doesn't have the "putIfAbsent"
like the Java5 class
http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/ConcurrentMap.html#putIfAbsent(K,%20V)

Which also supports configurable "partitions"
http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/ConcurrentHashMap.html#ConcurrentHashMap(int,%20float,%20int)

[clebert.suconic]

I understood that BeanLockManager needs to have some sort of atomicy, and because of that it would need these synchronized blocks.

Using ConcurrentHashMap from oswego would solve the partitions problem but we wouldn't have this level of atomicity. Is that correct?

 HashMap mapInUse = getHashMap(id);

 synchronized (mapInUse)
 {
 BeanLock lock = (BeanLock) mapInUse.get(id);
 if (lock!=null)
 {
 lock.addRef();
 return lock;
 }
 }

 BeanLock lock2 = (BeanLock)createLock(id);
 synchronized(mapInUse)
 {
 BeanLock lock = (BeanLock) mapInUse.get(id);
 // in case of bad luck, this might happen
 if (lock != null)
 {
 lock.addRef();
 return lock;
 }
 mapInUse.put(id, lock2);
 lock2.addRef();
 return lock2;
 }

[adrian.brock]

That is correct. There is no atomic operation in oswego's concurrenthashmap
that lets you putIfNotPresent() which is the code you are doing in the second
atomic block.

[adrian.brock]

HashMap mapInUse = getHashMap(id);

You also need to make sure this has putIfNotPresent() semantics.
We can't have two hashmaps for the same id getting created concurrently.

Thread 1: getHashMap(1); => map1
Thread 2: getHashMap(1); => map2 (overwrites map1)

Thread 1: map1.get(1) == null
Thread 2: map2.get(1) == null

Thread 1: new Lock(1) => lock1
Thread 2: new Lock(1) => lock2

Thread 1: map1.put(1, lock1);
Thread 2: map2.put(1, lock2);

And you now have two locks for the same id. Not what you want!

[clebert.suconic]

I'm using the hashCode to decide what HashMap to use. As hashCode is supposed to be always the same for a given ID, I think this would be okay.

 private HashMap getHashMap(Object id)
 {
 final int mapInUse = id.hashCode()%NUMBER_OF_INSTANCES;
 if (mapInUse>0)
 {
 return map[mapInUse];
 }
 else
 {
 return map[mapInUse*-1];
 }
 }

What you think?

[adrian.brock]

Yes it is ok. But only because you precontructed the

map[]

if you had been "lazily" constructing these elements it wouldn't be ok.

[clebert.suconic]

I found another problem with this, maybe that's the real problem of this lock contention.

org.jboss.util.id.UID (The ID used by SFBs) is always returning the same hashCode().

With this always returning the same hashCode, a HashMap will take longer time time to go through all the instances with equals operations:


From org.jboss.util.id.UID:

 /**
 * Return the hash code of this UID.
 *
 * @return The hash code of this UID.
 */
 public int hashCode() {
 return (int)((time ^ id) >> 32);
 }

 /**
 * Checks if the given object is equal to this UID.
 *
 * @param obj Object to test equality with.
 * @return True if object is equal to this UID.
 */
 public boolean equals(final Object obj) {
 if (obj == this) return true;

 if (obj != null && obj.getClass() == getClass()) {
 UID uid = (UID)obj;

 return
 uid.time == time &&
 uid.id == id;
 }

 return false;
 }

As changing hashCode's implementation won't affect serialization contract, and it was possibly broken before, can we implement it properly?


Clebert Suconic

[clebert.suconic]

I created a main() to show the behavior of these UIDs:

public static void main(String arg[])
 {
 for (int i=0;i<200;i++)
 {
 UID uid = new UID();
 try
 {
 Thread.sleep(50);
 } catch (InterruptedException e)
 {
 e.printStackTrace();
 }
 System.out.println("uid.hash=" + uid.hashCode() + " hash%40=" + (uid.hashCode()%40));
 }
 }

Here is the output:

uid.hash=264 hash%40=24
uid.hash=264 hash%40=24
uid.hash=264 hash%40=24
uid.hash=264 hash%40=24