[RFC][plugin]: fix fixme
Denis Lila
dlila at redhat.com
Thu Mar 31 09:44:15 PDT 2011
Also, I forgot to say:
the performance benefit of avoiding all these toString()
and concatenation-through-+ calls shouldn't be underestimated.
And I ran the tests with both patches applied. Everything was
ok.
Regards,
Denis.
----- Original Message -----
> 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
More information about the distro-pkg-dev
mailing list