Author: peter_firmstone Date: Mon May 20 13:15:41 2013 New Revision: 1484470
URL: http://svn.apache.org/r1484470 Log: Outrigger refactoring, increased use of synchronized for atomic method mutations and use of ConcurrentLinkedQueue to replace FastList and ConcurrentHashMap where it reduces synchronization complexity while retaining atomicity. Removed: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/FastList.java river/jtsk/skunk/qa_refactor/trunk/test/src/com/sun/jini/outrigger/FastListTest.java Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/AvailabilityRegistrationWatcher.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/BaseHandle.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandle.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleHashDesc.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleTmplDesc.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolder.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolderSet.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryRep.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/StoredResource.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TemplateHandle.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TransitionWatchers.java river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/WatchersForTemplateClass.java Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/AvailabilityRegistrationWatcher.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/AvailabilityRegistrationWatcher.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/AvailabilityRegistrationWatcher.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/AvailabilityRegistrationWatcher.java Mon May 20 13:15:41 2013 @@ -41,7 +41,7 @@ abstract class AvailabilityRegistrationW * The current expiration time of the registration * Protected, but only for use by subclasses. */ - volatile long expiration; + long expiration; /** * The UUID that identifies this registration @@ -55,7 +55,7 @@ abstract class AvailabilityRegistrationW * Only for use by subclasses. * Should not be changed. */ - volatile MarshalledObject handback; + MarshalledObject handback; /** * <code>true</code> if client is interested @@ -71,7 +71,7 @@ abstract class AvailabilityRegistrationW * Protected, but only for use by subclasses. * Should not be changed. */ - volatile long eventID; + long eventID; /** * The current sequence number. @@ -83,7 +83,7 @@ abstract class AvailabilityRegistrationW * watcher. */ private final Set<TemplateHandle> owners = new java.util.HashSet<TemplateHandle>(); - private volatile boolean removed = false; + private boolean removed = false; /** * The OutriggerServerImpl we are part of. @@ -173,8 +173,7 @@ abstract class AvailabilityRegistrationW // lock before checking the time and so we can update currentSeqNum synchronized (this) { - if (removed) - return; // Must have been removed + if (removed) return; // Must have been removed if (now > expiration) { doneFor = true; @@ -187,8 +186,7 @@ abstract class AvailabilityRegistrationW } } - if (doneFor) - cancel(); + if (doneFor) cancel(); } /** @@ -237,14 +235,13 @@ abstract class AvailabilityRegistrationW /** * Set the expiration time of this object. This method may be * called before <code>setTemplateHandle</code> is called. - * Assumes locking is handled by the caller. * @param newExpiration The expiration time. */ - public void setExpiration(long newExpiration) { + public final synchronized void setExpiration(long newExpiration) { expiration = newExpiration; } - public long getExpiration() { + public final synchronized long getExpiration() { return expiration; } @@ -286,12 +283,10 @@ abstract class AvailabilityRegistrationW final Set<TemplateHandle> owners; OutriggerServerImpl serv; synchronized (this) { - if (removed) - return false; // already removed + if (removed) return false; // already removed // Is this a force, or past our expiration? - if (!doIt && (now < expiration)) - return false; // don't remove, not our time + if (!doIt && (now < expiration)) return false; // don't remove, not our time owners = new java.util.HashSet<TemplateHandle>(this.owners); // Don't need to clone this.owners.clear(); @@ -349,7 +344,7 @@ abstract class AvailabilityRegistrationW throws UnknownEventException, IOException, ClassNotFoundException { boolean doneFor = false; - + RemoteEvent event; synchronized (AvailabilityRegistrationWatcher.this) { if (removed) return; // Our registration must been @@ -365,10 +360,11 @@ abstract class AvailabilityRegistrationW cancel(); return; } - } - getListener(preparer).notify( - new OutriggerAvailabilityEvent(source, eventID, ourSeqNumber, - handback, isVisible, rep)); + event = new OutriggerAvailabilityEvent(source, eventID, ourSeqNumber, + handback, isVisible, rep); + }// end sync(AvailabilityRegistrationWatcher.this) + // Overridden keep out of sync block. + getListener(preparer).notify( event ); } public void cancelRegistration() { Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/BaseHandle.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/BaseHandle.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/BaseHandle.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/BaseHandle.java Mon May 20 13:15:41 2013 @@ -23,8 +23,9 @@ package com.sun.jini.outrigger; * @author Sun Microsystems, Inc. * */ -class BaseHandle extends FastList.Node { - private final EntryRep rep; // the rep this handle manages +class BaseHandle { + private final EntryRep rep; // the rep this handle manages + private boolean removed = false; /** * Create a new handle @@ -44,6 +45,20 @@ class BaseHandle extends FastList.Node { public String classFor() { return rep.classFor(); } + + public synchronized boolean removed(){ + return removed; + } + + /** + * + * @return true if this is the first time remove is called. + */ + public synchronized boolean remove(){ + boolean result = !removed; + removed = true; + return result; + } } Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandle.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandle.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandle.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandle.java Mon May 20 13:15:41 2013 @@ -137,7 +137,7 @@ class EntryHandle extends BaseHandle imp */ EntryHandle(EntryRep rep, TransactableMgr mgr, EntryHolder holder) { super(rep); - hash = (rep != null ? hashFor(rep, rep.numFields()) : -1); + hash = (rep != null ? hashFor(rep, rep.numFields())[0] : -1); if (mgr == null) { txnState = null; } else { @@ -166,9 +166,9 @@ class EntryHandle extends BaseHandle imp * * @see #hashFor(EntryRep,int,EntryHandleHashDesc) */ - static long hashFor(EntryRep rep, int numFields) { - return hashFor(rep, numFields, null); - } +// static long hashFor(EntryRep rep, int numFields) { +// return hashFor(rep, numFields, null); +// } /** * Calculate the hash for a particular entry, assuming the given @@ -181,15 +181,20 @@ class EntryHandle extends BaseHandle imp * @see #hashFor(EntryRep,int) * @see #descFor(EntryRep,int) * @see EntryHandleHashDesc + * @return long[4] containing the hash, bitsPerField, fieldsInHash and mask + * in that order. */ - private static long - hashFor(EntryRep rep, int numFields, EntryHandleHashDesc hashDesc) + private static long [] + hashFor(EntryRep rep, int numFields) { - if (rep == null || numFields == 0) - return 0; - + long [] result = new long [4]; + if (rep == null || numFields == 0) return result; + + /** Number of bits allocated in the hash for each field */ int bitsPerField = Math.max(64 / numFields, 4); // at least 4 bits + /** How many fields are used in the hash? */ int fieldsInHash = 64 / bitsPerField; // max fields used + /** A mask with the lower <code>bitsPerField</code> bits set */ long mask = // per-field bit mask 0xffffffffffffffffL >>> (64 - bitsPerField); long hash = 0; // current hash value @@ -200,15 +205,12 @@ class EntryHandle extends BaseHandle imp // set the appropriate rep of the overall hash for the field's hash for (int i = 0; i < endField; i++) hash |= (hashForField(rep, i) & mask) << (i * bitsPerField); - - // If someone wants to remember these results, fill 'em in - if (hashDesc != null) { - hashDesc.bitsPerField = bitsPerField; - hashDesc.fieldsInHash = fieldsInHash; - hashDesc.mask = mask; - } - - return hash; + + result [0] = hash; + result [1] = bitsPerField; + result [2] = fieldsInHash; + result [3] = mask; + return result; } /** @@ -217,21 +219,29 @@ class EntryHandle extends BaseHandle imp * @see EntryHandleTmplDesc */ static EntryHandleTmplDesc descFor(EntryRep tmpl, int numFields) { - EntryHandleHashDesc hashDesc = new EntryHandleHashDesc(); - EntryHandleTmplDesc tmplDesc = new EntryHandleTmplDesc(); +// EntryHandleHashDesc hashDesc = new EntryHandleHashDesc(); + EntryHandleTmplDesc tmplDesc; // Get the hash and the related useful information - tmplDesc.hash = hashFor(tmpl, numFields, hashDesc); + long [] hashDesc = hashFor(tmpl, numFields); + long hash = hashDesc[0]; + long bitsPerField = hashDesc [1]; + long fieldsInHash = hashDesc [2]; + long mask = hashDesc [3]; + + long tmplMask = 0; // Create the mask to mask away wildcard fields - for (int i = 0; i < hashDesc.fieldsInHash; i++) { + for (int i = 0; i < fieldsInHash; i++) { // If this field is one we have a value for, set bits in the mask if (i < tmpl.numFields() && tmpl.value(i) != null) - tmplDesc.mask |= (hashDesc.mask << (i * hashDesc.bitsPerField)); + tmplMask |= (mask << (i * bitsPerField)); } // Ensure that the non-value fields are masked out - tmplDesc.hash &= tmplDesc.mask; + hash &= tmplMask; + + tmplDesc = new EntryHandleTmplDesc(hash, tmplMask); return tmplDesc; } Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleHashDesc.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleHashDesc.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleHashDesc.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleHashDesc.java Mon May 20 13:15:41 2013 @@ -18,12 +18,15 @@ package com.sun.jini.outrigger; /** + * No Longer required, dead code. + * * Used to hold the description of the parameters of the hash. * * @author Sun Microsystems, Inc. * * @see EntryHandle * @see EntryHandle#hashFor(EntryRep,int,EntryHandleHashDesc) + * @deprecated */ class EntryHandleHashDesc { /** Number of bits allocated in the hash for each field */ Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleTmplDesc.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleTmplDesc.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleTmplDesc.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHandleTmplDesc.java Mon May 20 13:15:41 2013 @@ -27,14 +27,19 @@ package com.sun.jini.outrigger; */ class EntryHandleTmplDesc { /** The hash value for the template itself, already masked */ - volatile long hash; + final long hash; /** * The mask for EntryHandle's hash codes -- if <code>handle.hash & * mask != tmplDesc.hash</code>, then the template doesn't match the * object held by the handle. */ - volatile long mask; + final long mask; + + EntryHandleTmplDesc (long hash, long mask){ + this.hash = hash; + this.mask = mask; + } public String toString() { return "0x" + Long.toHexString(hash) + " & 0x" + Long.toHexString(mask); Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolder.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolder.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolder.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolder.java Mon May 20 13:15:41 2013 @@ -20,14 +20,17 @@ package com.sun.jini.outrigger; import java.util.Hashtable; import java.util.Iterator; import java.util.Map; +import java.util.Queue; import java.util.Set; import java.util.WeakHashMap; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.logging.Level; import java.util.logging.Logger; import net.jini.core.entry.UnusableEntryException; import net.jini.core.transaction.CannotJoinException; import net.jini.core.transaction.server.TransactionConstants; +import net.jini.id.Uuid; /** * <code>EntryHolder</code>s hold all the entries of a exact given @@ -40,14 +43,16 @@ import net.jini.core.transaction.server. */ class EntryHolder implements TransactionConstants { /** The list that holds the handles */ - private final FastList<EntryHandle> contents = new FastList<EntryHandle>(); +// private final FastList<EntryHandle> contents = new FastList<EntryHandle>(); + + private final Queue<EntryHandle> content = new ConcurrentLinkedQueue<EntryHandle>(); /** * The map of cookies to handles, shared with the * <code>EntryHolderSet</code> and every other * <code>EntryHolder</code>. */ - private final Hashtable idMap; + private final Map<Uuid, EntryHandle> idMap; /** The server we are working for */ private final OutriggerServerImpl space; @@ -67,7 +72,7 @@ class EntryHolder implements Transaction * <code>EntryHolderSet</code> so that there is one table that can * map ID to <code>EntryRep</code> */ - EntryHolder(OutriggerServerImpl space, Hashtable idMap) { + EntryHolder(OutriggerServerImpl space, Map<Uuid,EntryHandle> idMap) { this.space = space; this.idMap = idMap; } @@ -123,7 +128,7 @@ class EntryHolder implements Transaction EntryHandleTmplDesc desc = null; long startTime = 0; - for (EntryHandle handle : contents) { + for (EntryHandle handle : content) { if (startTime == 0) { // First time through @@ -162,7 +167,7 @@ class EntryHolder implements Transaction void dump(String name) { try { System.out.println(name); - for (EntryHandle handle : contents) + for (EntryHandle handle : content) { EntryRep rep = handle.rep(); System.out.println(" " + rep + ", " + rep.entry()); @@ -278,15 +283,12 @@ class EntryHolder implements Transaction Set conflictSet, Set lockedEntrySet, Map provisionallyRemovedEntrySet) { - if (handle.removed()) - return false; - synchronized (handle) { + if (handle.removed()) return false; // get rid of stale entries if (isExpired(time, handle)) return false; - if (handle.removed()) // oh, well -- someone got it first - return false; + if (handle.isProvisionallyRemoved()) { if (provisionallyRemovedEntrySet != null) provisionallyRemovedEntrySet.put(handle, null); @@ -412,10 +414,12 @@ class EntryHolder implements Transaction // that is the only way knownMgr return true, and // managed() false) assert txn == null; - if (handle.removed() || handle.isProvisionallyRemoved()) - // Someone got to it first - return false; - handle.provisionallyRemove(); + synchronized (handle){ + if (handle.removed() || handle.isProvisionallyRemoved()) + // Someone got to it first + return false; + handle.provisionallyRemove(); + } } } } else { @@ -500,10 +504,7 @@ class EntryHolder implements Transaction * null if the list is empty. */ private EntryHandle getContentsHead(){ - for(EntryHandle head : contents){ - return head; - } - return null; + return content.peek(); } /** @@ -546,10 +547,8 @@ class EntryHolder implements Transaction if (head != null && head != handle) rep.shareWith(head.rep()); - if (txn != null) - txn.add(handle); - - contents.add(handle); + if (txn != null) txn.add(handle); + content.add(handle); idMap.put(rep.getCookie(), handle); } @@ -584,7 +583,7 @@ class EntryHolder implements Transaction SimpleRepEnum(TransactableMgr mgr) { this.mgr = mgr; startTime = System.currentTimeMillis(); - contentsIterator = contents.iterator(); + contentsIterator = content.iterator(); } // inherit doc comment from superclass @@ -598,9 +597,11 @@ class EntryHolder implements Transaction * Skip over handles which are either removed or unable to * perform a READ operation. */ - if (handle.canPerform(mgr, TransactableMgr.READ) - && !isExpired(startTime, handle) && !handle.removed()) { - return handle.rep(); + synchronized (handle){ + if (handle.canPerform(mgr, TransactableMgr.READ) + && !isExpired(startTime, handle) && !handle.removed()) { + return handle.rep(); + } } } @@ -687,7 +688,7 @@ class EntryHolder implements Transaction this.txn = txn; this.takeThem = takeThem; this.now = now; - contentsIterator = contents.iterator(); + contentsIterator = content.iterator(); } /** @@ -784,10 +785,9 @@ class EntryHolder implements Transaction * and matches one or more of the templates */ private boolean handleMatch(EntryHandle handle) { - if (handle.removed()) - return false; - - for (int i=0; i<tmpls.length; i++) { + if (handle.removed()) return false; + int length = tmpls.length; + for (int i=0; i<length; i++) { final EntryRep tmpl = tmpls[i]; final EntryHandleTmplDesc desc = descs[i]; @@ -813,10 +813,12 @@ class EntryHolder implements Transaction * <code>false</code> otherwise. */ boolean remove(EntryHandle h, boolean recovery) { - assert (recovery || Thread.holdsLock(h)); - - final boolean ok = contents.remove(h); - h.removalComplete(); + boolean ok = false; + synchronized (h){ + ok =content.remove(h); + assert h.remove(); + h.removalComplete(); + } if (ok) { idMap.remove(h.rep().getCookie()); /* This may cause an ifExists query to be resolved, @@ -839,7 +841,7 @@ class EntryHolder implements Transaction * is also the rep's <code>EntryHandle</code>. */ private EntryHandle handleFor(EntryRep rep) { - return (EntryHandle) idMap.get(rep.getCookie()); + return idMap.get(rep.getCookie()); } /** @@ -852,11 +854,11 @@ class EntryHolder implements Transaction // removed ("reaped") from the collection. long now = System.currentTimeMillis(); - for(EntryHandle handle : contents){ + for(EntryHandle handle : content){ // Don't try to remove things twice - if (handle.removed()) { - continue; - } +// if (handle.removed()) { +// continue; +// } // Calling isExpired() will both make the check and remove it // if necessary. @@ -865,7 +867,7 @@ class EntryHolder implements Transaction // This provides the FastList with an opportunity to actually // excise the items identified as "removed" from the list. - contents.reap(); +// contents.reap(); } } Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolderSet.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolderSet.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolderSet.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryHolderSet.java Mon May 20 13:15:41 2013 @@ -24,6 +24,8 @@ import java.util.Collection; import net.jini.core.lease.Lease; import net.jini.id.Uuid; import com.sun.jini.landlord.LeasedResource; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; /** * A set of <code>EntryHolder</code> objects for a given space. @@ -33,11 +35,11 @@ import com.sun.jini.landlord.LeasedResou * @see EntryHolder * @see OutriggerServerImpl */ -class EntryHolderSet { - private final Map holders = new java.util.HashMap(); +final class EntryHolderSet { + private final ConcurrentMap<String,EntryHolder> holders = new ConcurrentHashMap<String,EntryHolder>(); // Map of LeaseDescs indexed by the cookie of the underlying // LeasedResource. - private final Hashtable idMap = new Hashtable(); + private final Map<Uuid, EntryHandle> idMap = new Hashtable<Uuid, EntryHandle>(); private final OutriggerServerImpl space; @@ -62,15 +64,14 @@ class EntryHolderSet { * * @see #holderFor(EntryRep) */ - synchronized EntryHolder holderFor(String className) { - synchronized (holders) { - EntryHolder holder = (EntryHolder) holders.get(className); - if (holder == null) { - holder = new EntryHolder(space, idMap); - holders.put(className, holder); - } - return holder; - } + EntryHolder holderFor(String className) { + EntryHolder holder = holders.get(className); + if (holder == null) { + holder = new EntryHolder(space, idMap); + EntryHolder exists = holders.putIfAbsent(className, holder); + if (exists != null) holder = exists; + } + return holder; } LeasedResource getLeasedResource(Uuid cookie) { @@ -85,7 +86,7 @@ class EntryHolderSet { * with it. */ EntryHandle handleFor(Object cookie) { - return (EntryHandle)idMap.get(cookie); + return idMap.get(cookie); } /** @@ -93,14 +94,8 @@ class EntryHolderSet { * Assumes the caller holds the lock on handle */ void remove(EntryHandle handle) { - final EntryHolder holder; - synchronized (holders) { - holder = (EntryHolder) holders.get(handle.rep().classFor()); - } - - if (holder == null) - return; - + final EntryHolder holder = holders.get(handle.rep().classFor()); + if (holder == null) return; holder.remove(handle, false); } @@ -124,7 +119,7 @@ class EntryHolderSet { } for (int i=0; i<content.length; i++) { - content[i].reap(); + content[i].reap(); } } } Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryRep.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryRep.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryRep.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/EntryRep.java Mon May 20 13:15:41 2013 @@ -64,6 +64,11 @@ import net.jini.space.JavaSpace; class EntryRep implements StorableResource, LeasedResource, Serializable { static final long serialVersionUID = 3L; + // Synchronization isn't used where volatile access would be atomic. + // External operations should synchronize if atomicicity is required for + // multiple operations. + // Synchronization is used where multiple fields are accessed or one field + // is accessed more than once to ensure atomicity. /** * The fields of the entry in marshalled form. Use <code>null</code> * for <code>null</code> fields. @@ -134,7 +139,7 @@ class EntryRep implements StorableResour * Each entry is usable insofar as the codebase under which it was * written is usable. */ - void shareWith(EntryRep other) { + synchronized void shareWith(EntryRep other) { className = other.className; superclasses = other.superclasses; hashes = other.hashes; @@ -391,55 +396,64 @@ class EntryRep implements StorableResour */ Entry entry() throws UnusableEntryException { ObjectInputStream objIn = null; + String className = ""; // set before any exception can be thrown. try { ArrayList badFields = null; ArrayList except = null; - - realClass = ClassLoading.loadClass(codebase, className, - null, integrity, null); - - if (findHash(realClass, false).longValue() != hash) - throw throwNewUnusableEntryException( - new IncompatibleClassChangeError(realClass + " changed")); - - Entry entryObj = (Entry) realClass.newInstance(); - - Field[] fields = getFields(realClass); - - /* - * Loop through the fields, ensuring no primitives and - * checking for wildcards. - */ - int nvals = 0; // index into this.values[] - for (int i = 0; i < fields.length; i++) { - Throwable nested = null; - try { - if (!usableField(fields[i])) - continue; - - final MarshalledInstance val = values[nvals++]; - Object value = (val == null ? null : val.get(integrity)); - fields[i].set(entryObj, value); - } catch (Throwable e) { - nested = e; - } - - if (nested != null) { // some problem occurred - if (badFields == null) { - badFields = new ArrayList(fields.length); - except = new ArrayList(fields.length); - } - badFields.add(fields[i].getName()); - except.add(nested); - } - } + Entry entryObj = null; + int valuesLength = 0; + int nvals = 0; // index into this.values[] + + synchronized (this){ + className = this.className; + realClass = ClassLoading.loadClass(codebase, className, + null, integrity, null); + + if (findHash(realClass, false).longValue() != hash) + throw throwNewUnusableEntryException( + new IncompatibleClassChangeError(realClass + " changed")); + + entryObj = (Entry) realClass.newInstance(); + + Field[] fields = getFields(realClass); + + /* + * Loop through the fields, ensuring no primitives and + * checking for wildcards. + */ + + int fLength = fields.length; + valuesLength = values.length; + for (int i = 0; i < fLength; i++) { + Throwable nested = null; + try { + if (!usableField(fields[i])) + continue; + + final MarshalledInstance val = values[nvals++]; + Object value = (val == null ? null : val.get(integrity)); + fields[i].set(entryObj, value); + } catch (Throwable e) { + nested = e; + } + + if (nested != null) { // some problem occurred + if (badFields == null) { + badFields = new ArrayList(fLength); + except = new ArrayList(fLength); + } + badFields.add(fields[i].getName()); + except.add(nested); + } + } + } /* See if any fields have vanished from the class, * because of the hashing this should never happen but * throwing an exception that provides more info * (instead of AssertionError) seems harmless. */ - if (nvals < values.length) { + if (nvals < valuesLength) { throw throwNewUnusableEntryException( entryObj, // should this be null? null, // array of bad-field names @@ -450,7 +464,7 @@ class EntryRep implements StorableResour " since this EntryRep was created") }); } - + // if there were any bad fields, throw the exception if (badFields != null) { String[] bf = @@ -517,45 +531,47 @@ class EntryRep implements StorableResour EntryRep other = (EntryRep) o; - // If we're not the same class then we can't be equal - if (hash != other.hash) - return false; - - /* Paranoid check just to make sure we can't get an - * IndexOutOfBoundsException. Should never happen. - */ - if (values.length != other.values.length) - return false; - - /* OPTIMIZATION: - * If we have a case where one element is null and the corresponding - * element within the object we're comparing ourselves with is - * non-null (or vice-versa), we can stop right here and declare the - * two objects to be unequal. This is slightly faster than checking - * the bytes themselves. - * LOGIC: They've both got to be null or both have got to be - * non-null or we're out-of-here... - */ - for (int i = 0; i < values.length; i++) { - if ((values[i] == null) && (other.values[i] != null)) - return false; - if ((values[i] != null) && (other.values[i] == null)) - return false; - } - - /* The most expensive tests we save for last. - * Because we've made the null/non-null check above, we can - * simplify our comparison here: if our element is non-null, - * we know the other value is non-null, too. - * If any equals() calls from these element comparisons come - * back false then return false. If they all succeed, we fall - * through and return true (they were equal). - */ - for (int i = 0; i < values.length; i++) { - // Short-circuit evaluation if null, compare otherwise. - if (values[i] != null && !values[i].equals(other.values[i])) - return false; - } + synchronized (this){ + // If we're not the same class then we can't be equal + if (hash != other.hash) + return false; + + /* Paranoid check just to make sure we can't get an + * IndexOutOfBoundsException. Should never happen. + */ + if (values.length != other.values.length) + return false; + + /* OPTIMIZATION: + * If we have a case where one element is null and the corresponding + * element within the object we're comparing ourselves with is + * non-null (or vice-versa), we can stop right here and declare the + * two objects to be unequal. This is slightly faster than checking + * the bytes themselves. + * LOGIC: They've both got to be null or both have got to be + * non-null or we're out-of-here... + */ + for (int i = 0; i < values.length; i++) { + if ((values[i] == null) && (other.values[i] != null)) + return false; + if ((values[i] != null) && (other.values[i] == null)) + return false; + } + + /* The most expensive tests we save for last. + * Because we've made the null/non-null check above, we can + * simplify our comparison here: if our element is non-null, + * we know the other value is non-null, too. + * If any equals() calls from these element comparisons come + * back false then return false. If they all succeed, we fall + * through and return true (they were equal). + */ + for (int i = 0; i < values.length; i++) { + // Short-circuit evaluation if null, compare otherwise. + if (values[i] != null && !values[i].equals(other.values[i])) + return false; + } + } return true; } @@ -600,27 +616,28 @@ class EntryRep implements StorableResour * been called. */ void pickID() { - if (id != null) - throw new IllegalStateException("pickID called more than once"); - id = UuidFactory.generate(); + synchronized (this){ + if (id != null) + throw new IllegalStateException("pickID called more than once"); + id = UuidFactory.generate(); + } } /** * Return the <code>MarshalledObject</code> for the given field. */ public MarshalledInstance value(int fieldNum) { - return values[fieldNum]; + return values[fieldNum]; } /** * Return the number of fields in this kind of entry. */ public int numFields() { - if (values != null) { - return values.length; - } else { - return 0; - } + synchronized (this){ + if (values != null) return values.length; + } + return 0; } /** @@ -667,17 +684,18 @@ class EntryRep implements StorableResour //Note: If this object is the MatchAny template then // return true (all entries match MatchAny) - if (EntryRep.isMatchAny(this)) - return true; - - for (int f = 0; f < values.length; f++) { - if (values[f] == null) { // skip wildcards - continue; - } - if (!values[f].equals(other.values[f])) { - return false; - } - } + synchronized (this){ + if (EntryRep.isMatchAny(this)) return true; + + for (int f = 0; f < values.length; f++) { + if (values[f] == null) { // skip wildcards + continue; + } + if (!values[f].equals(other.values[f])) { + return false; + } + } + } return true; // no mismatches, so must be OK } @@ -693,13 +711,14 @@ class EntryRep implements StorableResour if (otherClass.equals(matchAnyClassName())) // The other is a null template, all entries are at least entry. return true; - - if (className.equals(otherClass)) - return true; - for (int i = 0; i < superclasses.length; i++) - if (superclasses[i].equals(otherClass)) - return true; - return false; + synchronized (this){ + if (className.equals(otherClass)) + return true; + for (int i = 0; i < superclasses.length; i++) + if (superclasses[i].equals(otherClass)) + return true; + return false; + } } /** Comparator for sorting fields. Cribbed from Reggie */ @@ -786,7 +805,7 @@ class EntryRep implements StorableResour // ------------------------------------- // inherit doc comment - public void store(ObjectOutputStream out) throws IOException { + public synchronized void store(ObjectOutputStream out) throws IOException { final long bits0; final long bits1; if (id == null) { @@ -808,7 +827,7 @@ class EntryRep implements StorableResour } // inherit doc comment - public void restore(ObjectInputStream in) + public synchronized void restore(ObjectInputStream in) throws IOException, ClassNotFoundException { final long bits0 = in.readLong(); Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/StoredResource.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/StoredResource.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/StoredResource.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/StoredResource.java Mon May 20 13:15:41 2013 @@ -33,6 +33,10 @@ public interface StoredResource { * Restore the state of a <code>StorableResource</code>. The resource * to be restored will have its expiration set before this method * returns. + * + * If this method returned a new StorableResource instead of mutating + * the passed in StorableResource as a side effect, the implementation + * could be thread safe and immutable. * * @see LogOps#renewOp * Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TemplateHandle.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TemplateHandle.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TemplateHandle.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TemplateHandle.java Mon May 20 13:15:41 2013 @@ -17,8 +17,10 @@ */ package com.sun.jini.outrigger; +import java.util.ArrayList; import java.util.Set; import java.util.Iterator; +import java.util.List; import java.util.Vector; /** @@ -31,7 +33,7 @@ class TemplateHandle extends BaseHandle * A cache of the <code>EntryHandleTmplDesc</code> indexed * by the number of fields. */ - final private Vector descs = new Vector(); + final private ArrayList<EntryHandleTmplDesc> descs = new ArrayList<EntryHandleTmplDesc>(); /** @@ -45,7 +47,7 @@ class TemplateHandle extends BaseHandle * changing <code>FastList</code> to support overlapping traversals * of different lists from the same thread.) */ - final private Set watchers = new java.util.HashSet(); + final private Set<TransitionWatcher> watchers = new java.util.HashSet<TransitionWatcher>(); /** * The <code>WatchersForTemplateClass</code> this object @@ -66,24 +68,39 @@ class TemplateHandle extends BaseHandle */ //!! Since the mask/hash algorithm tops out at a certain number of fields, //!! we could avoid some overhead by topping out at the same count. - EntryHandleTmplDesc descFor(int numFields) { + EntryHandleTmplDesc descFor(int index) { /* Since setSize can truncate, test and set need to be atomic. * Hold the lock after setting the size so don't calculate * a given desc more than once (though that is only an optimization) */ synchronized (descs) { - // Make sure descs is big enough - if (numFields >= descs.size()) - descs.setSize(numFields + 1); + + int size = descs.size(); // Do we have a cached value? - EntryHandleTmplDesc desc = - (EntryHandleTmplDesc)descs.elementAt(numFields); + EntryHandleTmplDesc desc = null; + if (index < size) desc = descs.get(index); if (desc == null) { // None in cache, calculate one - desc = EntryHandle.descFor(rep(), numFields); - descs.setElementAt(desc, numFields); + desc = EntryHandle.descFor(rep(), index); + if (index == size){ + descs.add(desc); + } + else if (index < size){ + descs.set(index, desc); + } + // Make sure descs is big enough and pad with null if + // fields are added out of order. + else if (index > size) { + descs.ensureCapacity(index + 1); + int difference = index - size; + for (int i = 0; i < difference; i++){ + descs.add(null); + } + descs.add(desc); + } + assert descs.indexOf(desc) == index; } return desc; } @@ -110,6 +127,8 @@ class TemplateHandle extends BaseHandle if (watcher == null) throw new NullPointerException("Watcher can not be null"); + // If thread holds lock during removal TransitionWatcher cannot be added + // so this assertion is unnecessary. assert !removed() : "Added watcher to a removed TemplateHandle"; watchers.add(watcher); } Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TransitionWatchers.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TransitionWatchers.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TransitionWatchers.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/TransitionWatchers.java Mon May 20 13:15:41 2013 @@ -19,7 +19,10 @@ package com.sun.jini.outrigger; import java.util.Map; import java.util.Collection; +import java.util.Iterator; import java.util.SortedSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; /** * Given an <code>EntryHandle</code> who's entry is making a @@ -38,7 +41,8 @@ class TransitionWatchers { * A map from class names to <code>WatchersForTemplateClass</code> * objects */ - final private Map holders = new java.util.HashMap(); + final private ConcurrentMap<String,WatchersForTemplateClass> holders + = new ConcurrentHashMap<String,WatchersForTemplateClass>(); /** The server we are working for */ final private OutriggerServerImpl server; @@ -82,15 +86,12 @@ class TransitionWatchers { void add(TransitionWatcher watcher, EntryRep template) { // Get/create the appropriate WatchersForTemplateClass final String className = template.classFor(); - WatchersForTemplateClass holder; - synchronized (holders) { - holder = (WatchersForTemplateClass) holders.get(className); - if (holder == null) { - holder = new WatchersForTemplateClass(this); - holders.put(className, holder); - } - } - + WatchersForTemplateClass holder = holders.get(className); + if (holder == null) { + holder = new WatchersForTemplateClass(this); + WatchersForTemplateClass existed = holders.putIfAbsent(className, holder); + if (existed != null) holder = existed; + } // Add the watcher to the WatchersForTemplateClass holder.add(watcher, template); } @@ -129,39 +130,24 @@ class TransitionWatchers { WatchersForTemplateClass holder; /* Collect all the watchers looking for the exact class of the - * transitioned entry. Sync both to protect holder, and - * to make sure we do at least one sync so we see any - * watchers added before this call. + * transitioned entry. */ - synchronized (holders) { - holder = (WatchersForTemplateClass)holders.get(className); - } + holder = holders.get(className); - if (holder != null) - holder.collectInterested(rslt, transition, ordinal); + if (holder != null) holder.collectInterested(rslt, transition, ordinal); // Get all the templates that are super classes of className final String[] superclasses = rep.superclasses(); - for (int i=0; i<superclasses.length; i++) { - synchronized (holders) { - holder = - (WatchersForTemplateClass)holders.get(superclasses[i]); - } + for (int i=0; i<superclasses.length; i++) { + holder = holders.get(superclasses[i]); if (holder != null) holder.collectInterested(rslt, transition, ordinal); } // Including those registered for the null template final String nullClass = EntryRep.matchAnyEntryRep().classFor(); - synchronized(holders) { - holder = - (WatchersForTemplateClass)holders.get(nullClass); - } - - if (holder!=null){ - holder.collectInterested(rslt, transition, ordinal); - } - + holder = holders.get(nullClass); + if (holder!=null) holder.collectInterested(rslt, transition, ordinal); return rslt; } @@ -171,6 +157,7 @@ class TransitionWatchers { * <code>FastList</code>s. */ void reap() { + //Old comment related to cloning holders.values(). /* This could take a while, instead of blocking all other * access to handles, clone the contents of handles and * iterate down the clone (we don't do this too often and @@ -179,17 +166,13 @@ class TransitionWatchers { * clone would probably work well since we don't add (and * never remove) elements that often.) */ - final WatchersForTemplateClass content[]; - synchronized (holders) { - final Collection values = holders.values(); - content = new WatchersForTemplateClass[values.size()]; - values.toArray(content); - } - + final long now = System.currentTimeMillis(); - for (int i=0; i<content.length; i++) { - content[i].reap(now); - } + + Iterator<WatchersForTemplateClass> watchers = holders.values().iterator(); + while (watchers.hasNext()){ + watchers.next().reap(now); + } } /** Modified: river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/WatchersForTemplateClass.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/WatchersForTemplateClass.java?rev=1484470&r1=1484469&r2=1484470&view=diff ============================================================================== --- river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/WatchersForTemplateClass.java (original) +++ river/jtsk/skunk/qa_refactor/trunk/src/com/sun/jini/outrigger/WatchersForTemplateClass.java Mon May 20 13:15:41 2013 @@ -17,7 +17,9 @@ */ package com.sun.jini.outrigger; +import java.util.Queue; import java.util.Set; +import java.util.concurrent.ConcurrentLinkedQueue; /** * Holds a collection of <code>TemplateHandle</code>s who's templates @@ -28,7 +30,9 @@ import java.util.Set; */ class WatchersForTemplateClass { /** All the templates we know about */ - private final FastList<TemplateHandle> contents = new FastList<TemplateHandle>(); +// private final FastList<TemplateHandle> contents = new FastList<TemplateHandle>(); + + private final Queue<TemplateHandle> content = new ConcurrentLinkedQueue<TemplateHandle>(); /** The object we are inside of */ private final TransitionWatchers owner; @@ -65,7 +69,7 @@ class WatchersForTemplateClass { * if we have more than one with the same template. It * is bad if we add the watcher to a removed handle. */ - for(TemplateHandle handle : contents) { + for(TemplateHandle handle : content) { if (template.equals(handle.rep())) { synchronized (handle) { if (!handle.removed()) { @@ -99,12 +103,13 @@ class WatchersForTemplateClass { /* First add handle to contents so handle is fully initialized * before we start to pass it around use */ - contents.add(handle); + content.add(handle); if (watcher.addTemplateHandle(handle)) { handle.addTransitionWatcher(watcher); } else { // watcher is already dead, undo adding handle - contents.remove(handle); + content.remove(handle); + handle.remove(); } } } @@ -133,18 +138,18 @@ class WatchersForTemplateClass { * the changed entry and if they do ask them to * put the appropriate watchers in the set. */ - for (TemplateHandle handle : contents) + for (TemplateHandle handle : content) { // See the if handle mask is incompatible - EntryHandleTmplDesc desc = handle.descFor(repNumFields); + EntryHandleTmplDesc desc = handle.descFor(repNumFields); // final - if ((entryHash & desc.mask) != desc.hash) - continue; + if ((entryHash & desc.mask) != desc.hash) continue; - if (handle.matches(rep)) { - if (handle.removed()) //no sync, ok if we check a removed one - continue; - handle.collectInterested(set, transition, ordinal); + if (handle.matches(rep)) { // rep access is synchronized + synchronized (handle){ + if (handle.removed()) continue; + handle.collectInterested(set, transition, ordinal); + } } } } @@ -168,7 +173,7 @@ class WatchersForTemplateClass { */ void reap(long now) { // First remove empty handles - for (TemplateHandle handle : contents) + for (TemplateHandle handle : content) { // Dump any expired watchers. handle.reap(now); @@ -179,7 +184,8 @@ class WatchersForTemplateClass { */ synchronized (handle) { if (handle.isEmpty()) { - contents.remove(handle); + content.remove(handle); + handle.remove(); continue; } } @@ -188,7 +194,7 @@ class WatchersForTemplateClass { /* This provides the FastList with an opportunity to actually * excise the items identified as "removed" from the list. */ - contents.reap(); +// contents.reap(); } }
