Fix JSObject.equals
Dr Andrew John Hughes
ahughes at redhat.com
Thu Mar 31 14:04:16 PDT 2011
On 16:53 Thu 31 Mar , Denis Lila wrote:
> Hi.
>
> In this e-mail:
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-March/013329.html
> I document the problems that JSObject.equals was causing
> (i.e. breaking reflexivity and calling JSObject.toString()).
> This patch fixes them. It does so by removing it
> altogether. This means we no longer call PluginDebug.debug.
>
> If anyone would miss it, I'm fine with putting it back
> in, and just doing
>
> public boolean equals(Object o) {
> PluginDebug.debug("equals called ", o);
> return this == o;
> }
>
> The other change is that with the new equals implementation,
> we can use ConcurrentHashMaps in PluginObjectStore, which
> allow us to remove synchronization from reads. An incomplete
> but rigorous (though not necessarily correct) proof of why
> ConcurrentHashMaps allow us to do that is here:
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-March/013360.html
>
> Thank you,
> Denis.
I think these should be separate patches.
With the PluginObjectStore, I really need some time to look over this. I'll
try and get to it on Monday.
The equals change seems correct as it'll revert to ==, where it has to be the
same instance of JSObject.
> diff -r 955da8a90b33 ChangeLog
> --- a/ChangeLog Thu Mar 31 15:15:29 2011 -0400
> +++ b/ChangeLog Thu Mar 31 16:45:06 2011 -0400
> @@ -1,3 +1,16 @@
> +2011-03-31 Denis Lila <dlila at redhat.com>
> +
> + * plugin/icedteanp/java/netscape/javascript/JSObject.java:
> + Fix comments, remove unused imports.
> + (equals): Remove. It was breaking the reflexivity of the
> + equals contract.
> + * plugin/icedteanp/java/sun/applet/PluginObjectStore.java:
> + Remove unused import, add ConcurrentHashMap import.
> + (objects, counts, identifiers): Make final ConcurrentHashMaps.
> + (getObject, getIdentifier, contains(Object), contains(int), dump):
> + Remove synchronization, since all they do is read, and now the maps
> + are thread safe.
> +
> 2011-03-31 Denis Lila <dlila at redhat.com>
>
> * plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> diff -r 955da8a90b33 plugin/icedteanp/java/netscape/javascript/JSObject.java
> --- a/plugin/icedteanp/java/netscape/javascript/JSObject.java Thu Mar 31 15:15:29 2011 -0400
> +++ b/plugin/icedteanp/java/netscape/javascript/JSObject.java Thu Mar 31 16:45:06 2011 -0400
> @@ -37,7 +37,7 @@
> *
> * ***** END LICENSE BLOCK ***** */
>
> -/* more doc todo:
> +/* more doc TODO:
> * threads
> * gc
> *
> @@ -47,10 +47,8 @@
> package netscape.javascript;
>
> import java.applet.Applet;
> -import java.security.AccessControlContext;
> import java.security.AccessControlException;
> import java.security.AccessController;
> -import java.security.BasicPermission;
>
> import sun.applet.PluginAppletViewer;
> import sun.applet.PluginDebug;
> @@ -193,8 +191,6 @@
> PluginAppletViewer.setSlot(internal, index, value);
> }
>
> - // TODO: toString, finalize.
> -
> /**
> * Removes a named member of a JavaScript object.
> */
> @@ -267,14 +263,4 @@
> PluginDebug.debug("JSObject.finalize ");
> PluginAppletViewer.JavaScriptFinalize(internal);
> }
> -
> - /**
> - * Override java.lang.Object.equals() because identity is not preserved
> - * with instances of JSObject.
> - */
> - public boolean equals(Object obj) {
> - PluginDebug.debug("JSObject.equals " + obj);
> -
> - return false;
> - }
> }
> diff -r 955da8a90b33 plugin/icedteanp/java/sun/applet/PluginObjectStore.java
> --- a/plugin/icedteanp/java/sun/applet/PluginObjectStore.java Thu Mar 31 15:15:29 2011 -0400
> +++ b/plugin/icedteanp/java/sun/applet/PluginObjectStore.java Thu Mar 31 16:45:06 2011 -0400
> @@ -37,16 +37,16 @@
>
> package sun.applet;
>
> -import java.util.HashMap;
> import java.util.Map;
> +import java.util.concurrent.ConcurrentHashMap;
>
> // Enums are the best way to implement singletons.
> enum PluginObjectStore {
> INSTANCE;
>
> - private HashMap<Integer, Object> objects = new HashMap<Integer, Object>();
> - private HashMap<Integer, Integer> counts = new HashMap<Integer, Integer>();
> - private HashMap<Object, Integer> identifiers = new HashMap<Object, Integer>();
> + private final ConcurrentHashMap<Integer, Object> objects = new ConcurrentHashMap<Integer, Object>();
> + private final ConcurrentHashMap<Integer, Integer> counts = new ConcurrentHashMap<Integer, Integer>();
> + private final ConcurrentHashMap<Object, Integer> identifiers = new ConcurrentHashMap<Object, Integer>();
> private final Object lock = new Object();
>
> private boolean wrapped = false;
> @@ -57,32 +57,24 @@
> }
>
> public Object getObject(Integer identifier) {
> - synchronized(lock) {
> - return objects.get(identifier);
> - }
> + return objects.get(identifier);
> }
>
> public Integer getIdentifier(Object object) {
> - synchronized(lock) {
> - if (object == null)
> - return 0;
> - return identifiers.get(object);
> - }
> + if (object == null)
> + return 0;
> + return identifiers.get(object);
> }
>
> public boolean contains(Object object) {
> - synchronized(lock) {
> - if (object == null)
> - return identifiers.containsKey(object);
> + if (object == null)
> + return identifiers.containsKey(object);
>
> - return false;
> - }
> + return false;
> }
>
> public boolean contains(int identifier) {
> - synchronized(lock) {
> - return objects.containsKey(identifier);
> - }
> + return objects.containsKey(identifier);
> }
>
> private boolean checkNeg() {
> @@ -131,11 +123,9 @@
> }
>
> public void dump() {
> - synchronized(lock) {
> - if (PluginDebug.DEBUG) {
> - for (Map.Entry<Integer, Object> e : objects.entrySet()) {
> - PluginDebug.debug(e.getKey(), "::", e.getValue());
> - }
> + if (PluginDebug.DEBUG) {
> + 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