[RFC][plugin]: fix fixme

[Denis Lila]

> 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. I
didn't synchronize the reads because a get of an object
and a put of that same object can't race. 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.

> 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.

Regards,
Denis.

----- Original Message -----
> On 09:58 Wed 30 Mar , Denis Lila wrote:
> > Hi.
> >
> > This patch implements the fixme in PluginObjectStore
> > using the "wrapped" idea in that comment. It also
> > synchronizes modifications to the 3 hash maps because,
> > afaics, they can be called concurrently.
> >
> > ChangeLog:
> > +2011-03-30 Denis Lila <dlila at redhat.com>
> > +
> > + * plugin/icedteanp/java/sun/applet/PluginObjectStore.java
> > + (wrapped): New static variable.
> > + (getNextID): New function.
> > + (reference): Using getNextID and synchronized.
> > + (unreference): Synchronized.
> > + (dump): Improve iteration.
> >
> > Any criticism is always welcome.
> >
> > Thank you,
> > Denis.
> 
> > diff -r 3bbc4314e02c ChangeLog
> > --- a/ChangeLog Tue Mar 29 10:24:31 2011 -0400
> > +++ b/ChangeLog Wed Mar 30 09:56:26 2011 -0400
> > @@ -1,3 +1,12 @@
> > +2011-03-30 Denis Lila <dlila at redhat.com>
> > +
> > + * plugin/icedteanp/java/sun/applet/PluginObjectStore.java
> > + (wrapped): New static variable.
> > + (getNextID): New function.
> > + (reference): Using getNextID and synchronized.
> > + (unreference): Synchronized.
> > + (dump): Improve iteration.
> > +
> >  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 09:56:26 2011 -0400
> > @@ -43,19 +43,7 @@
> >      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 boolean wrapped = false;
> >      private static int nextUniqueIdentifier = 1;
> >
> >      public Object getObject(Integer identifier) {
> > @@ -79,38 +67,51 @@
> >          return objects.containsKey(identifier);
> >      }
> >
> > + private int getNextID() {
> > + if (nextUniqueIdentifier == Integer.MAX_VALUE) {
> > + wrapped = true;
> > + nextUniqueIdentifier = 1;
> > + }
> > + if (wrapped)
> > + while(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));
> > + 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hg.diff
Type: text/x-patch
Size: 4911 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110330/6717d527/hg.diff