[icedtea-web] RFC: Patch to fix PR895
Deepak Bhole
dbhole at redhat.com
Mon Mar 12 11:20:23 PDT 2012
* Deepak Bhole <dbhole at redhat.com> [2012-03-12 14:13]:
> * Omair Majid <omajid at redhat.com> [2012-03-09 20:08]:
> > On 03/09/2012 05:19 PM, Deepak Bhole wrote:
> > > Hi,
> > >
> > > Attached patch fixes PR895:
> > > "IcedTea-Web searches for missing classes on each loadClass or findClass"
> > >
> > > http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=895
> >
> > The bug doesn't seem to have any additional information. Is there a test
> > case or a reproducer? Do you know why the server is being hammered? If
> > the actual issue is under our control (for example, we are attempting to
> > re-download stuff we already downloaded), it might be better to fix this
> > underlying issue.
> >
>
> Yes, this one:
> http://jdk6.java.net/nonav/plugin2/liveconnect/LiveConnectTests/
>
> Without the patch it takes 2+ minutes to run.. with patch, it is down to
> about 15 seconds.
>
> > > ChangeLog:
> > > 2012-03-09 Deepak Bhole <dbhole at redhat.com>
> > >
> > > * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: Added new
> > > notFoundClasses list.
> > > (loadClass): Record if a class is not found and on next call for that
> > > class, return immediately.
> > > (findClass): Same.
> > >
> >
> > Can you please include a test case for icedtea-web? I think it would be
> > nice if we could tell if/when this issue reappears later. I would be
> > fine with either unit tests or jnlp tests.
> >
>
> I am not sure if I can. It is really only an issue with the
> CodeBaseClassLoader, since looking into local jars multiple times has
> comparatively trivial cost. CBCL is only used with the plug-in and not
> webstart and I don't think the plugin test framework is fully in place
> yet. Jiri?
>
> > Some other thoughts:
> >
> > Won't this patch break applications that use the following pattern?
> > (Assume the application has enough permissions to download the jar)
> > - Try and load a class
> > - If that fails, download a new jar
> > - Add new jar to classpath
> > - Try to load the class again.
> >
> > I am not sure how many applications do this; perhaps it's a non-issue.
> >
> > The pattern of adding a new line before every return looks scary. I am
> > quite sure the next person to touch this method and add a new return
> > will miss adding the code to update notFoundClasses. How about something
> > like this instead:
> >
> > - rename findClass to findClassNoCacheCheck (or a better name ;) )
> > - add a new method findClass():
> >
> > protected Class findClass(String name) throws ClassNotFoundException {
> > if (notFoundClasses.contains(name)) {
> > throw new ClassNotFoundException;
> > }
> >
> > try {
> > Class klass = findClassNoCacheCheck(name);
> > return klass;
> > } catch (ClassNotFoundException cnfe) {
> > notFoundClasses.add(name);
> > throw cnfe;
> > }
> > }
> >
>
> While writing the above, I realized that it is really just CBCL that we
> want to fix. What about moving the logic just into CBCL? See attached
> patch.
>
annndddd... here is the patch
Deepak
> In most cases, notFoundResources.get() will return null so I don't think
> that the Array comparisons will add a significant delay.
>
> Thanks,
> Deepak
-------------- next part --------------
diff -r d2aff3800f4f netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Mar 08 15:54:39 2012 +0100
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Mon Mar 12 14:01:52 2012 -0400
@@ -37,6 +37,7 @@
import java.security.PrivilegedAction;
import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.Enumeration;
import java.util.HashMap;
@@ -47,9 +48,11 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.Vector;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
+
import net.sourceforge.jnlp.AppletDesc;
import net.sourceforge.jnlp.ApplicationDesc;
import net.sourceforge.jnlp.DownloadOptions;
@@ -1874,6 +1877,11 @@
JNLPClassLoader parentJNLPClassLoader;
+ /**
+ * Classes that are not found, so that findClass can skip them next time
+ */
+ ConcurrentHashMap<String, URL[]> notFoundResources = new ConcurrentHashMap<String, URL[]>();
+
public CodeBaseClassLoader(URL[] urls, JNLPClassLoader cl) {
super(urls);
parentJNLPClassLoader = cl;
@@ -1885,8 +1893,18 @@
}
@Override
- public Class<?> findClass(String name) throws ClassNotFoundException {
- return super.findClass(name);
+ public Class<?> findClass(String name) throws ClassNotFoundException {
+
+ // If we have searched this path before, don't try again
+ if (Arrays.equals(super.getURLs(), notFoundResources.get(name)))
+ throw new ClassNotFoundException(name);
+
+ try {
+ return super.findClass(name);
+ } catch (ClassNotFoundException cnfe) {
+ notFoundResources.put(name, super.getURLs());
+ throw cnfe;
+ }
}
/**
@@ -1913,17 +1931,41 @@
@Override
public Enumeration<URL> findResources(String name) throws IOException {
+
+ // If we have searched this path before, don't try again
+ if (Arrays.equals(super.getURLs(), notFoundResources.get(name)))
+ return (new Vector<URL>(0)).elements();
+
if (!name.startsWith("META-INF")) {
- return super.findResources(name);
+ Enumeration<URL> urls = super.findResources(name);
+
+ if (!urls.hasMoreElements()) {
+ notFoundResources.put(name, super.getURLs());
+ }
+
+ return urls;
}
+
return (new Vector<URL>(0)).elements();
}
@Override
public URL findResource(String name) {
+
+ // If we have searched this path before, don't try again
+ if (Arrays.equals(super.getURLs(), notFoundResources.get(name)))
+ return null;
+
if (!name.startsWith("META-INF")) {
- return super.findResource(name);
+ URL url = super.findResource(name);
+
+ if (url == null) {
+ notFoundResources.put(name, super.getURLs());
+ }
+
+ return url;
}
+
return null;
}
}
More information about the distro-pkg-dev
mailing list