[RFC]: Linkage Error fix.

[Deepak Bhole]

* 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.
> 

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.

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

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).

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?

> +        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.

>          if (result == null) {
>              throw new ClassNotFoundException(name);
>          }
>  
>          return result;
> +        }

Indentation is incorrect above.

Cheers,
Deepak