LockInterceptor.lock() creates Fqn's unecessarily
Hi;
As a follow-up to my last post on TreeCache.findNode() creating Fqn's unecessarily, a similar (but slightly different) problem exists in LockInterceptor.lock().
The problem with LockInterceptor.lock() is that the tmp_fqn is being created on every iteration of the for/next loop (using treeSize).
This seems over-eager, as the only time tmp_fqn is needed, is within an if condition branch in which the child_node being locked must be null, and the createIfNotExists flag must be set to true.
While the cost of constructing each Fqn is small, the LockInterceptor.lock() method gets hit pretty hard, and in deeply nested trees (like mine), that means a lot of iterations through the for/next loop creating a ton of Fqn's without much benefit. I don't think this problem is limited to my application as I would suspect the ratio between node creation and node access to be fairly high (in favour of node access).
In any case, a JProbe analysis of my app shows that LockInterceptor.lock() is now by far the leading cause of Fqn creation, and Fqn's are second from the top in terms of object creation - hence a major cause of woe in terms of gc time (BTW, EvictedEventNode gets first place, but that's another story).
So, to resolve this issue I propose two changes:.
1. Only construct the tmp_fqn within the if(createIfNotExists) block.
2. Rename Fqn.add(Object) to Fqn.addRelativeName(Object) and make it public. This allows an Fqn path to be efficiently extended. We need this to allow us to efficiently create Fqn's when needed in LockInterceptor.lock(). There are other places as well where this kind of call would be useful.
I'm happy to submit a bug on this through Jira, but thought I'd post my thoughts here first and get some feedback. Below are the code changes to LockInterceptor.lock(). I have not posted the changes to Fqn as they are not complicated, but am happy to do so if you'd like.
Thanks in advance,
Brian.
private void lock(Fqn fqn, GlobalTransaction gtx, int lock_type, List locks, boolean recursive,
long lock_timeout, boolean createIfNotExists)
throws TimeoutException, LockingException, InterruptedException {
Node n, child_node=null;
Object child_name, owner=gtx != null? gtx : (Object)Thread.currentThread();
// FIXME: the next line can be removed, it's no longer necessary
//Fqn tmp_fqn=new Fqn(); int treeNodeSize;
boolean acquired=false;
IsolationLevel isolation_level;
if(fqn == null) {
log.error("fqn is null - this should not be the case");
return;
}
if((treeNodeSize=fqn.size()) == 0)
return;
isolation_level=cache.getIsolationLevelClass();
if(isolation_level == IsolationLevel.NONE)
lock_type=Node.LOCK_TYPE_NONE;
n=cache.getRoot();
for(int i=0; i < treeNodeSize; i++) {
child_name=fqn.get(i);
// FIXME: the next line can be removed - it's no longer necessary
//tmp_fqn=new Fqn(tmp_fqn, child_name); if(createIfNotExists)
create_lock.acquire();
try {
child_node=n.getChild(child_name);
if(child_node == null) {
if(createIfNotExists) {
// FIXME: start of patch block
// build a new Fqn for this child
// based on the path traversed so far
final Fqn tmp_fqn = new Fqn();
for (int namePos = 0; namePos <= i; namePos++) {
tmp_fqn.addRelativeName(fqn.get(namePos));
}
// FIXME: end of patch block
child_node=n.createChild(child_name, tmp_fqn, n);
if(log.isTraceEnabled())
log.trace("created child " + child_name);
if(gtx != null) {
// add the node name to the list maintained for the current tx
// (needed for abort/rollback of transaction)
// FIXME: removed tmp_fqn.clone() no longer necessary
// since tmp_fqn is local to this block and will not be modified
//
cache.addNode(gtx, tmp_fqn); }
create_lock.release();
cache.notifyNodeCreated(tmp_fqn);
}
else {
if(log.isTraceEnabled())
log.trace("failed finding child " + child_name + " of node " + n.getFqn());
return;
}
}
}
finally {
if(create_lock.holds() > 0)
create_lock.release();
}
if(lock_type == Node.LOCK_TYPE_NONE) {
// acquired=false;
n=child_node;
continue;
}
else {
if(lock_type == Node.LOCK_TYPE_WRITE && i == (treeNodeSize - 1)) {
acquired=child_node.acquire(owner, lock_timeout, Node.LOCK_TYPE_WRITE);
}
else {
acquired=child_node.acquire(owner, lock_timeout, Node.LOCK_TYPE_READ);
}
}
if(acquired) {
if(gtx != null) {
// add the lock to the list of locks maintained for this transaction
// (needed for release of locks on commit or rollback)
cache.getTransactionTable().addLock(gtx, child_node.getLock());
}
else {
IdentityLock l=child_node.getLock();
if(!locks.contains(l))
locks.add(l);
}
}
if(recursive && i == (treeNodeSize - 1)) {
Set acquired_locks=child_node.acquireAll(owner, lock_timeout, lock_type);
if(acquired_locks.size() > 0) {
if(gtx != null) {
cache.getTransactionTable().addLocks(gtx, acquired_locks);
}
else {
locks.addAll(acquired_locks);
}
}
}
n=child_node;
}
}