[RFC][plugin]: fix fixme

[Dr Andrew John Hughes]

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);
> +            }
>          }

Are these the only use of objects, counts and identifiers? If so, this looks ok.

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

This needs to be synchronized on objects too to produce a consistent state.

>          }
>      }
>  }


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