[RFC][plugin]: fix fixme
Dr Andrew John Hughes
ahughes at redhat.com
Thu Mar 31 11:59:02 PDT 2011
On 16:58 Wed 30 Mar , Denis Lila wrote:
> > Are these the only use of objects, counts and identifiers? If so, this
> > looks ok.
>
> They're not the only uses. They're the only writes.
Ok, where are the other uses? Just in that class?
> I
> didn't synchronize the reads because a get of an object
> and a put of that same object can't race.
How do you come to this conclusion? puts aren't atomic.
> Thinking
> about it more, I realize that was a mistake: if an
> object is being removed from the hash map at the same
> time as a get is executing, and if the two objects in
> question happen to be in the same bucket, that would be
> very bad.
>
> The simplest way to fix this would be to synchronize
> everything, but reads seem to be pretty common, so that
> might be bad.
>
> The attached patch uses ConcurrentHashMaps.
>
Are you sure you understand how these work?
reads aren't blocked and you're not using any of the atomic operations.
I really need to look at the whole usage of these maps to see the best
route forward.
> > This needs to be synchronized on objects too to produce a consistent
> > state.
>
> This doesn't really change anything. It only iterates
> through objects, but I guess it would generate nonsensical
> output if a race occurred, so I synchronized it too.
> I also skip it completely if !DEBUG, since it only did
> anything when debug mode was on.
>
If any write operation is taking place, it's going to produce
some inconsistent state depending on the point at which it's called.
Things are better with ConcurrentHashMap as the iterators are
weakly consistent.
I'm not sure objects is the right thing to synchronise on. It may be
better to use an Object instance as a lock.
> Regards,
> Denis.
>
> diff -r 3bbc4314e02c ChangeLog
> --- a/ChangeLog Tue Mar 29 10:24:31 2011 -0400
> +++ b/ChangeLog Wed Mar 30 17:01:22 2011 -0400
> @@ -1,3 +1,13 @@
> +2011-03-30 Denis Lila <dlila at redhat.com>
> +
> + * plugin/icedteanp/java/sun/applet/PluginObjectStore.java
> + (wrapped): New static variable.
> + (getNextID, checkNeg): New functions.
> + (reference): Using getNextID and synchronized.
> + (unreference): Synchronized.
> + (dump): Improve iteration and synchronized.
> + (objects, counts, identifiers): Make them ConcurrentHashMaps.
> +
> 2011-03-29 Denis Lila <dlila at redhat.com>
>
> * netx/net/sourceforge/jnlp/JNLPFile.java
> diff -r 3bbc4314e02c plugin/icedteanp/java/sun/applet/PluginObjectStore.java
> --- a/plugin/icedteanp/java/sun/applet/PluginObjectStore.java Tue Mar 29 10:24:31 2011 -0400
> +++ b/plugin/icedteanp/java/sun/applet/PluginObjectStore.java Wed Mar 30 17:01:22 2011 -0400
> @@ -37,25 +37,14 @@
>
> package sun.applet;
>
> -import java.util.*;
> +import java.util.Map;
> +import java.util.concurrent.ConcurrentHashMap;
>
> public class PluginObjectStore {
> - private static HashMap<Integer, Object> objects = new HashMap<Integer, Object>();
> - private static HashMap<Integer, Integer> counts = new HashMap<Integer, Integer>();
> - private static HashMap<Object, Integer> identifiers = new HashMap<Object, Integer>();
> - // FIXME:
> - //
> - // IF uniqueID == MAX_LONG, uniqueID =
> - // 0 && wrapped = true
> - //
> - // if (wrapped), check if
> - // objects.get(uniqueID) returns null
> - //
> - // if yes, use uniqueID, if no,
> - // uniqueID++ and keep checking
> - // or:
> - // stack of available ids:
> - // derefed id -> derefed id -> nextUniqueIdentifier
> + private static ConcurrentHashMap<Integer, Object> objects = new ConcurrentHashMap<Integer, Object>();
> + private static ConcurrentHashMap<Integer, Integer> counts = new ConcurrentHashMap<Integer, Integer>();
> + private static ConcurrentHashMap<Object, Integer> identifiers = new ConcurrentHashMap<Object, Integer>();
> + private static boolean wrapped = false;
> private static int nextUniqueIdentifier = 1;
>
> public Object getObject(Integer identifier) {
> @@ -79,38 +68,57 @@
> return objects.containsKey(identifier);
> }
>
> + private static boolean checkNeg() {
> + if (nextUniqueIdentifier < 1) {
> + wrapped = true;
> + nextUniqueIdentifier = 1;
> + }
> + return wrapped;
> + }
> + private int getNextID() {
> + while (checkNeg() && objects.containsKey(nextUniqueIdentifier))
> + nextUniqueIdentifier++;
> + return nextUniqueIdentifier++;
> + }
> +
> public void reference(Object object) {
> - Integer identifier = identifiers.get(object);
> - if (identifier == null) {
> - objects.put(nextUniqueIdentifier, object);
> - counts.put(nextUniqueIdentifier, 1);
> - identifiers.put(object, nextUniqueIdentifier);
> - nextUniqueIdentifier++;
> - } else {
> - counts.put(identifier, counts.get(identifier) + 1);
> + synchronized(objects) {
> + Integer identifier = identifiers.get(object);
> + if (identifier == null) {
> + int next = getNextID();
> + objects.put(next, object);
> + counts.put(next, 1);
> + identifiers.put(object, next);
> + } else {
> + counts.put(identifier, counts.get(identifier) + 1);
> + }
> }
> }
>
> public void unreference(int identifier) {
> - Integer currentCount = counts.get(identifier);
> - if (currentCount == null) {
> - return;
> - }
> - if (currentCount == 1) {
> - Object object = objects.get(identifier);
> - objects.remove(identifier);
> - counts.remove(identifier);
> - identifiers.remove(object);
> - } else {
> - counts.put(identifier, currentCount - 1);
> + synchronized(objects) {
> + Integer currentCount = counts.get(identifier);
> + if (currentCount == null) {
> + return;
> + }
> + if (currentCount == 1) {
> + Object object = objects.get(identifier);
> + objects.remove(identifier);
> + counts.remove(identifier);
> + identifiers.remove(object);
> + } else {
> + counts.put(identifier, currentCount - 1);
> + }
> }
> }
>
> public void dump() {
> - Iterator i = objects.keySet().iterator();
> - while (i.hasNext()) {
> - Object key = i.next();
> - PluginDebug.debug(key + "::" + objects.get(key));
> + if (PluginDebug.DEBUG) {
> + synchronized(objects) {
> + for (Map.Entry<Integer,Object> e : objects.entrySet()) {
> + PluginDebug.debug(e.getKey() + "::" + e.getValue());
> + }
> + }
> }
> }
> }
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list