[RFC][plugin]: fix fixme

[Denis Lila]

> I don't think the whole concept of wrapped is needed at all. Even at
> an
> allocation rate of 1000 objects/second (which is extremely high.. the
> best I have seen anything do is maybe 30-50), it would take
> MAX_INT/(1000*3600*365) = 1.65 years to fill.. it is very likely that
> the browser will be restarted or the JVM will be updated atleast once
> within that time frame.

Ok. I just wanted to get rid of the FIXME :D
Now that it's done, should I revert it?

> Why not synchronize the whole function? The whole body is synchronized
> on a static variable anyway. I think the multi-threaded map access is
> a genuine potential problem. Making the reference and unreference
> functions synchronized and keeping everything else as is should be
> enough to address that.

If we sync on the function, then wouldn't we be opening up a deadlock
possibility? If some other thread has a lock on "this" but is waiting
on a lock that the current thread owns, we will be deadlocked if we
try to go into a synchronized method. Right now this can't happen
because we never use the monitor of a PluginObjectStore variable, but
if someday someone does, they'd be in trouble. "objects" on the
other hand is a private field, and we can only lock it from within
the class, so it's obvious that there's no bad locking order.

The other reason is that the reference and unreference methods are
non-static methods that modify static objects. If they were made
synchronized multiple threads could still concurrently modify
the static hash maps, as long as they do it through different
PluginObjectStore variables (right now we only create one POS
object, but POS is not a singleton, so we reserve the right
to create more. As long as that's a possibility, we can't
make "reference" and "unreference" synchronized).

Regards,
Denis.

----- Original Message -----
> * Denis Lila <dlila at redhat.com> [2011-03-30 09:59]:
> > 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.
> 
> >
> > + 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);
> > + }
> >          }
> 
> 

> 
> Cheers,
> Deepak
> 
> >      }
> >
> >      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());
> >          }
> >      }
> >  }