[RFC][plugin]: fix fixme

[Deepak Bhole]

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


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.

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


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.

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