[RFC]: Linkage Error fix.

[Denis Lila]

> Did you look into how URLCloassLoader and others mitigate the problem?
> If I am not mistaken, all class load requests go through
> JNLPClassLoader,
> and synchronizing on each such call can get expensive.

I did after I read this e-mail. URLClassloader does not
override loadClass. ClassLoader implements it, and it's
just a synchronized method, which is exactly what I was
trying to avoid in this patch.
As for synchronization costs: we have a race condition.
Some synchronization is unavoidable. The best thing for
performance would be to maximize concurrency and making
the method synchronized would completely lock it down. 
My fix avoids that by having a different lock for each 
class name being loaded.

> I understand the use of Hash* for lookup speed, but seems like a lot
> of un-necessary object creation is going on here (1 per loaded class).

Maybe so, but I don't think this is hurting us in any
way. Object instances are very small and if I'm not
mistaken their allocation is just bumping a pointer.
Their GC costs practically nothing, since if they don't
make it into the CHM they'll die as soon as they're
created and they won't make it into the old generation.

> Perhaps only keep an entry for classes loaded by *this* loader by
> removing the map entry (outside the synchronized block) if its loader
> was the parent?

I'm not sure I understand this. I'll think about it, but
I will say that the fix right now is 3 lines, and complicating
the logic to any degree to eliminate one "new Object()" per
call would be a premature optimization (especially considering
how expensive the findLoadedClassAll(name) call is compared to
"new Object()").

> > + synchronized(loadedClassNames.get(name)) {
> >          Class<?> result = findLoadedClassAll(name);
> >
> >          // try parent classloader
> > @@ -1003,7 +1009,7 @@
> >          // validPackage(name);
> >
> >          // search this and the extension loaders
> > - if (result == null)
> > + if (result == null) {
> >              try {
> >                  result = loadClassExt(name);
> >              } catch (ClassNotFoundException cnfe) {
> > @@ -1060,12 +1066,14 @@
> >                      }
> >                  }
> >              }
> > + }
> >
> 
> I take it the extra brackets are just to make it look cleaner? If so,
> thats fine.

Yeah. It's a pet peeve of mine to put "{}" on all if/for/while. I try
to ignore it, but in this case we had a bracketless "if" with a 30+
line body. It was confusing.

> >          if (result == null) {
> >              throw new ClassNotFoundException(name);
> >          }
> >
> >          return result;
> > + }
> 
> Indentation is incorrect above.

That was intentional. I was trying to make it easier to read.
Of course, I'll fix it when I push.

Regards,
Denis.

----- Original Message -----
> * Denis Lila <dlila at redhat.com> [2011-04-15 15:43]:
> > Hi.
> >
> > Andrew Su pointed out this bug to me a few days ago.
> > In the page
> > http://www-archive.mozilla.org/quality/browser/front-end/testcases/oji/objtest2.html
> > one of the applets does not load once in a while (it happens
> > very often on my machine (>80% of the time)).
> >
> > A LinkageError is thrown.
> >
> > The attached patch fixes it.
> >
> > The problem was that "JNLPClassloader.loadClass" was being called
> > explicitly by the applet instance creation process. That page loads
> > the same applet many times, so there was a race condition where
> > findLoadedClassAll(name) returned null (meaning the class has not
> > been loaded yet), so then we moved on to actually loading it.
> > However
> > by the time we got to the loading code another applet had finished
> > loading the class. Trying to define a class twice in the same
> > classloader is an error.
> >
> > The patch fixes it by not allowing concurrent loading of classes
> > with the same name. Another fix would be to just synchronize
> > the method body on a static lock, but that might be too slow.
> > The only thing I'm worried about with the attached patch is that
> > the map might get a bit big after a while. Is this a realistic
> > concern?
> > If yes, it could be fixed by changing the hash map to <Integer,
> > Object>,
> > and instead of putting (name, new Object()) in it, I could
> > put(name.hashCode()%83, new Object()). That way key,value pairs
> > would
> > never take up more than about 2Kb, and with 83 locks, I don't think
> > we would have to worry about reduced concurrency.
> >
> 

> 
> OTOH, if URLClassLoader is doing the same, we are not impacting the
> performance any further so it'd be okay.
> 
> As for the patch, comments below:
> 
> >       */
> > + private static ConcurrentMap<String, Object> loadedClassNames =
> > new ConcurrentHashMap<String, Object>();
> >      public Class<?> loadClass(String name) throws
> >      ClassNotFoundException {
> >
> > + loadedClassNames.putIfAbsent(name, new Object());
> 

> 


> 

> 
> Cheers,
> Deepak