Custom ClassLoader for indexing classes #85

rpau · 2018-04-11T18:01:33Z

@cal101 feel free to review it :)

coveralls · 2018-04-11T18:03:33Z

Coverage increased (+0.1%) to 86.043% when pulling 720e80c on rpau-index into 4788033 on master.

rpau · 2018-04-11T18:03:34Z

@cal101 what I have realized is that using ASM (bytecode parser) is probably much faster than using reflection, specially to read the class definition. Now, the critical point is addType in TypesLoaderVisitor.

https://github.com/EsotericSoftware/reflectasm

cal101 · 2018-04-12T08:01:52Z

Reflectasm sounds cool but is reflection really a (measured?) problem?
Or do you think about avoiding class loading via class loser and replace that with reflectasm?
I wonder what that means for access of depended classes.

Since I got some strange results when profiling the class loader related code when I profiled that I used some simple timer based counting System.currentTime*. I found the results more reliable.

Regarding the review I will try to find a slot.

cal101 · 2018-04-12T17:27:16Z

src/main/java/org/walkmod/javalang/compiler/types/ClassPathTree.java

+import java.io.File;
+import java.util.*;
+
+public class ClassPathTree {


The add method looks strange to me. I suggest writing a test that shows application of the class. Especially dealing with different depths ok key pathes.
I would call the keys parameter a keyPath.
I wonder if the outcome of adding

A/B/C

A/D/E
is correct according to the assumed expected behavior.
All fields seem to be candidates for "final".
Telling the syntax of the key would be nice: absolute file path? Relative file path?

Fixed in e36e5a8

cal101 · 2018-04-12T17:51:31Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+    }
+
+    public IndexedURLClassPath(URL[] urls) {
+        super(urls);


Make copy of array?

why do you suggest to copy this array?

Deep mistrust ;-) The array is writable and we are creating an alias. I don't want to check the control flow to make sure all users behave and don't modify. Immutable data types or defensive copy make it possible to keep it private/unmodifyable. Parallel execution plans make that even more important.

I have marked it as final in 5e6fb89
The copy is created by who provides the array ;)

cal101 · 2018-04-12T17:54:10Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+
+
+    // Map from resource name to URLClassPath to delegate loading that resource to.
+    private final ClassPathTree index = new ClassPathTree("/", "/", new HashMap<String, ClassPathTree>(), null);


I expected this declaration at beginning of class.

cal101 · 2018-04-12T18:00:56Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+               if (!rootPath.endsWith(File.separator)) {
+                   rootPath = rootPath + File.separator;
+               }
+               addFilesToIndex(rootPath.length(), root, delegate);


Whats the urlclasspath definition for file archive?
In the past we had .zip accepted. Years ago but would check against the urlclassloader.

URLClasspath belongs to the "misc" classes of the JVM to load resources.
I have excluded zips because I did not about this, but I can include them as well.

I suggest checking how urlclassloader handles non-directories and do the same. If they check for suffixes we can do the same. But I expectvsomething different.

URLConnection urlc = url.openConnection();
InputStream is = urlc.getInputStream();
if (urlc instanceof JarURLConnection) {

is what urclassloader does.
I suggest trying to open each non-directory as jar and ignore errors if it's not a jar.

This is not right at all. The URLClassLoader is checking the permissions for the file with the approach that you pointed out.
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/net/URLClassLoader.java#458

However, here: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/misc/URLClassPath.java#URLClassPath.JarLoader

I don't see your point/contradiction.

In the JarLoader is not opening a connection, it is iterating the entries. Looking in detail in the code, I am starting to think that I should maintain and index in TypesLoaderVisitor but using the current URLClassLoader, which already is indexing the entries. Then, the pros are:

I do not depend on sun.miscpackage, which is not part of the JDK in Java 10.

I reduce the TypesLoader part because the classloader is not needed anymore for the changes that you see in the next PR

The cons are:

The indexation happens twice.

Nevermind. It not indexes the classpath at the begining. Every request is really expensive in the URLClassLoader class

cal101 · 2018-04-12T18:03:57Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+                maybeIndexResource(relPath, delegate);
+            }
+            File[] directoryEntries = f.listFiles();
+            assert(directoryEntries != null);


Throw ioexception instead of assert. See methods javadoc.

Ops it was because the code was initially copied from the pants project

cal101 · 2018-04-12T18:10:42Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+            // Callers may request the directory itself as a resource, and may
+            // do so with or without trailing slashes.  We do this in a while-loop
+            // in case the classpath element has multiple superfluous trailing slashes.
+            if (relPath.endsWith(File.separator)) {


Move before outer if to store only without trailing slashes. Strip trailingvslashes on lookup, too if not already done.
File separator is always char and has constants for string and char.

cal101 · 2018-04-12T18:22:38Z

src/main/java/org/walkmod/javalang/compiler/types/TypesLoaderVisitor.java

+    }
+
+    private void addTypes(List<String> files, List<SymbolAction> actions, Node node) {
+        Iterator<String> it = files.iterator();


Here and other places: why not just use for loop instead of explicit iterator?

cal101 · 2018-04-12T18:30:42Z

src/main/java/org/walkmod/javalang/compiler/types/TypesLoaderVisitor.java

+            } else {
+                classLoader = new CachedClassLoader(new IndexedURLClassLoader(cl));
+            }
+            SDKFiles = classLoader.getSDKContents("java.lang");


Do you care for avoiding statics at the moment or do you want deal with them later?
There is some kind of context missing that brackets an execution/run.
I am dreaming of parallel execution here...

Really good point about the parallel execution here :)

Well, I think that I cannot add more statics for now without overcomplicating the code. Indeed with visualVM now, the hotspot is the LoadStaticImportsAction because of the reflection usage there asking for the import class.

What do you mean with a missing context?

If you want to do parallel runs each run needs it's own set of mutable state.
Often ThreadLocals are used but I prefer explicit passing of a kind of "execution context" that stores data global for a run. For single threaded apps the "statics" provide that.

Cal, let's work in the parallel part in a different PR. I do not see how to do it easily with the current logic.

Sure. I don't want to work on it now. I just wanted to point out not working too much against it.

cal101

Check comments. Ignore where too picky... ;-)

rpau · 2018-04-12T19:49:05Z

@cal101 thanks for your comments. Now I will review and fix one by one :)

BTW: about the performance improvement replacing reflection by ASM: #86

rpau · 2018-04-13T17:44:05Z

@cal101 waiting for your approval ;)

cal101 · 2018-04-13T22:13:21Z

Will look at it after some sleep, garden work, shopping, working and driving. Maybe even some snack inbetween ;-)

cal101

Minor style stuff, see comments.

cal101 · 2018-04-14T16:11:06Z

src/main/java/org/walkmod/javalang/compiler/types/CachedClassLoader.java

@@ -14,35 +14,52 @@
 */
 package org.walkmod.javalang.compiler.types;

-import java.util.HashMap;
-import java.util.Map;
+import java.util.*;


Style: wild or no wild?

It has been automatically performed by Intellij. I will removed it.

cal101 · 2018-04-14T16:12:16Z

src/main/java/org/walkmod/javalang/compiler/types/CachedClassLoader.java

-public class SymbolTypesClassLoader extends ClassLoader {
+public class CachedClassLoader extends ClassLoader {
+
+    public static Map<String, Class<?>> PRIMITIVES = new HashMap<String, Class<?>>();


Collections.unmodifyableMap?

cal101 · 2018-04-14T16:13:56Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassLoader.java

+
+    /* The search path for classes and resources */
+    private IndexedURLClassPath ucp;
+


Style: Move to top of class?

cal101 · 2018-04-14T16:19:32Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.*;


cal101 · 2018-04-14T16:20:13Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+
+    private final URL[] urls;
+    private int lastIndexed = 0;
+    private static URL RT_JAR;


cal101 · 2018-04-14T16:22:59Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+        // static block to resolve java.lang package classes
+        String[] bootPath = System.getProperties().get("sun.boot.class.path").toString()
+                .split(Character.toString(File.pathSeparatorChar));
+        for (String lib : bootPath) {


File.pathSeparator

cal101 · 2018-04-14T16:24:26Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+                .split(Character.toString(File.pathSeparatorChar));
+        for (String lib : bootPath) {
+            if (lib.endsWith("rt.jar")) {
+                File f = new File(lib);


Will that work for java 9?
Should we care now or later?

Well, I would care if rt is not part of java 9

The internal files rt.jar, tools.jar, and dt.jar have been removed. The content of these files is now stored in a more efficient format in implementation-private files in the lib directory. Class and resource files previously in tools.jar and dt.jar are now always visible via the bootstrap or application class loaders in a JDK image.

😱

I am going to create a Issue, because it is a different discussion.

rpau · 2018-04-17T16:14:54Z

@cal101 could you review the last changes and then approve if you consider? I think that all your styling comments have been addressed.

cal101

Mostly style issues.

cal101 · 2018-04-17T16:27:35Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassPath.java

+   * @param relPath relative path
+   * @param delegate value to insert
+   */
+    private void maybeIndexResource(String relPath, URL delegate) {


What I meant was:

String cleanRelPath = stripTrailingFileSeparators(); if (!index.containsKey(cleanRelPath)) { index.put(cleanRelPath, delegate); }

And using the same strip-Method for lookup method of index, too.
Seems fishy for me to add multiples here but don't strip on lookup.

Theoretically, you could have multiple classes from different jars under the same package and the JDK uses the first that appears in the classpath.

cal101 · 2018-04-17T16:27:45Z

src/main/java/org/walkmod/javalang/compiler/types/PathTree.java

+
+
+import java.io.File;
+import java.util.*;


cal101 · 2018-04-17T16:28:38Z

src/main/java/org/walkmod/javalang/compiler/types/TypesLoaderVisitor.java

-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;


cal101 · 2018-04-17T16:29:31Z

src/main/java/org/walkmod/javalang/compiler/types/IndexedURLClassLoader.java

+public class IndexedURLClassLoader extends ClassLoader {
+
+    /* The search path for classes and resources */
+    private IndexedURLClassPath ucp;


Can be final.

Custom ClassLoader for indexing classes

67ea64a

cal101 reviewed Apr 12, 2018

View reviewed changes

cal101 requested changes Apr 12, 2018

View reviewed changes

rpau force-pushed the rpau-index branch from 5bf9ab2 to 67ea64a Compare April 12, 2018 19:46

rpau force-pushed the rpau-index branch from 2553db3 to ce6f96c Compare April 13, 2018 13:20

Refactoring and Test for ClassPathTree

e36e5a8

rpau force-pushed the rpau-index branch from ce6f96c to e36e5a8 Compare April 13, 2018 13:27

Raquel Pau added 6 commits April 13, 2018 15:29

url array as final

5e6fb89

Moving index at the top of the class declaration

f385baa

Replacing assert by exception

a95af1e

replacing iterators by fors

ab5a243

Moving comment as javadoc

a825719

Removal of sun.misc classes

fb3d67d

cal101 previously approved these changes Apr 14, 2018

View reviewed changes

rpau added 2 commits April 17, 2018 18:00

fixup! Custom ClassLoader for indexing classes

465cde0

fix merge

4436022

rpau dismissed cal101’s stale review via 4436022 April 17, 2018 16:02

cal101 approved these changes Apr 17, 2018

View reviewed changes

fixup! fix merge

720e80c

rpau merged commit d7da813 into master Apr 17, 2018



		// Map from resource name to URLClassPath to delegate loading that resource to.
		private final ClassPathTree index = new ClassPathTree("/", "/", new HashMap<String, ClassPathTree>(), null);


		/* The search path for classes and resources */
		private IndexedURLClassPath ucp;

Custom ClassLoader for indexing classes #85

Custom ClassLoader for indexing classes #85

Conversation

rpau commented Apr 11, 2018

coveralls commented Apr 11, 2018 • edited Loading

rpau commented Apr 11, 2018

cal101 commented Apr 12, 2018

Choose a reason for hiding this comment

rpau Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpau Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cal101 Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

rpau Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cal101 left a comment • edited Loading

Choose a reason for hiding this comment

rpau commented Apr 12, 2018

rpau commented Apr 13, 2018

cal101 commented Apr 13, 2018

cal101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpau commented Apr 17, 2018

cal101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 11, 2018 •

edited

Loading

rpau Apr 13, 2018 •

edited

Loading

rpau Apr 13, 2018 •

edited

Loading

cal101 Apr 12, 2018 •

edited

Loading

rpau Apr 12, 2018 •

edited

Loading

cal101 left a comment •

edited

Loading