[RFC][plugin]: fix fixme
Denis Lila
dlila at redhat.com
Thu Mar 31 09:32:40 PDT 2011
Hi.
I ran the live connect tests on this patch (with the
ConcurrentHashMaps) yesterday and after the first 37
tests successfully completed, it just stopped making
progress. The bug was interesting, and here's what happened:
The code for HashMap.get has this line in it:
if (e.hash == hash && ((k = e.key) == key || key.equals(k)))
return e.value;
where key is the parameter we pass in, and e.key is the key
of an entry it is considering. A somewhat equivalent line in
ConcurrentHashMap.get is this:
if (e.hash == hash && key.equals(e.key)) {
They're identical, except ConcurrentHashMap doesn't have
the e.key==key optimization.
When PASC.handleMessage calls store.contains with a JSObject
argument. HashMap.get completely skips the JSObject.equals
call because e.key==key returns true and all is well.
When the same happens but we're using a ConcurrentHashMap
the optimization isn't there, so JSObject.equals gets
called, which is:
public boolean equals(Object obj) {
PluginDebug.debug("JSObject.equals " + obj);
return false;
}
There are two problems here, one in each line:
(1) obj.toString() is called before debug(). obj is a JSObject,
whose toString calls PAV.javascriptToString(long), which sends
a message to the C++ side and waits for an answer. For some
reason (not sure why), the C++ side never answers so this waits()
indefinitely, and this is why the tests stopped after the 37th.
(2) Even if the above wasn't a problem, there's still a bug
because of "return false", which will result in the object
not being found. This is a huge problem, because in PASC.handleMessage
we do things like:
store.reference(o);
store.getIdentifier(o);
all the time. We assume (justifiably) that if we put an object
in the store, the store won't say "it's not there" a few instructions
later. This assumption is wrong though, because the hash map
is not able to find the object we just put in, because equals
always returns false.
In other words, the whole correctness of the LiveConnect
implementation hinges on a minor optimization in HashMap.
posConcurrency.patch gets around these problems by
going back to HashMaps. This unfortunately means that we have
to synchronize the reads. The real fix would be to change
JSObject.equals so it doesn't break reflexivity (but that's a
separate issue, so it should be a different patch).
The second patch tries to avoid things like (1) above.
It's big, but the only real change is that
PluginDebug.debug(String) was changed to
PluginDebug.debug(Object...). The point of this was to avoid
implicit calls to toString() and String '+' until after we
check the DEBUG flag. Right now we have a lot of debug calls
and most of them do string concatenation and toString() calls,
even when it's unnecessary because DEBUG==false. In the worst case
we do a blocking toString with side effects, which is horrible.
This patch ensures that none of that happens until we've checked
that it actually needs to happen (unless we specifically demand
a toString() call by putting it in explicitly).
----- Original Message -----
> > 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: posConcurrency.patch
Type: text/x-patch
Size: 5486 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110331/a129acb0/posConcurrency.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pluginDebug.patch
Type: text/x-patch
Size: 38275 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110331/a129acb0/pluginDebug.patch
More information about the distro-pkg-dev
mailing list