RFR 8054717: SJavac should track changes in the public apis of classpath classes!

[Andreas Lundblad]

On Sat, Aug 09, 2014 at 12:22:29AM +0200, Fredrik Öhrström wrote:
>
> [...]
> 
> http://cr.openjdk.java.net/~ohrstrom/webrev-8054717-classpathdep/
> 
> https://bugs.openjdk.java.net/browse/JDK-8054717
> 
> //Fredrik


Overall I think this is good work and I don't see it as a "feature" but rather a necessity. I don't think the quality of the code is top-notch. Some things I'd prefer we address before pushing this patch, other things I don't mind dealing with myself in later cleanups.

My review follows:

----

JavacState.java:

> // The set of all classpath packages and their classes, 
> // for which either the timestamps or the pubapi have changed. 
> private Map<String,Set<String>> changedClasspathPackages;

Is String really the adequate type for keys/values here?

----

JavacState.java:

taintPackagesDependingOnChangedClasspathPackages deals with both parsing and the tainting logic. I suggest we do the parsing in one pass, and then decide what should be tainted.

----

Could you please elaborate on this?

> // With two threads compiling our sources, sources compiled by a second thread, might look like 
> // classpath dependencies to the first thread or vice versa. We cannot remove such fake classpath dependencies 
> // until the end of the compilation since the knowledge of what is compiled does not exist until now.

and this:

> // Also, if we doing an incremental compile, then references outside of the small recompiled set,
> // will also look like classpath deps, lets remove them as well.

I've tried to wrap my head around it without success.

(Edit: There's a note on concurrency towards the end of this mail.)

----

Compile.java: I suggest we import the classes that are currently fully qualified.

----

> /**
>  * Extract the pubapi for a class fn. If the class was fetched from an archive, 
>  * store the archive in the set archives.
>  */

The functionality of extracting the public API and the functionality for deciding what should end up in the 'archives' collection should probably be separated. The whole pattern of "storing return values in the arguments" (or having side effects in arguments in general) is a bit of a code smell. If possible, the method should be broken down to two separate methods, each one returning a separate result. Otherwise, I reccomend that we create a "Result" class to store the results of the operation.

----

> } catch (Exception e) {
>     e.printStackTrace(System.err);
> }

This looks dodgy to me. I think it's better to catch the exceptions we actually suspect could be thrown. (This rarely includes RuntimeExceptions I think.)

----

Package.java: I suggest we go for camel case instead of "pubapi_for_compiled_sources" etc.

----

Util.java:

> /**
>  * Extract the jar/zip/cym archive file name from a classfile tosttring.
>  */

How did we end up with a String? Why can't we hold on to the original data type? The whole c.startsWith("ZipFileIndexFileObject[") smells.

----

Util.java

> if (s == null) return deflt;

I don't think the settings-string should be null in the first place. Wherever the settings-string was parsed, it should probably default to the empty string instead.

----

Util.java:

> public static void addToMapSet(String k, String v, Map<String,Set<String>> m) {
>     Set<String> set = m.get(k);
>     if (set == null) {
>         set = new HashSet<String>();
>         m.put(k, set);
>     }
>     set.add(v);
> }

is probably better implemented as

public static <K, V> void addToMapSet(K k, V v, Map<K,Set<V>> m) {
    m.merge(k, Collections.singleton(v), Util::union);
}

Same goes for addToMapList

Also, I suggest we rename the methods to for instance multimapPut and multimapAppend.

----

A general remark: It's unfortunate that so much machinery is needed to find the file to which a type belongs. (Larger part of Compile.java.) However I don't see a better/more stable way of ensuring that the right answer is found. (In fact, I think the approach is quite clever.) As mentioned in some other email, this whole lookup procedure should probably move to the server in a thin-client approach (It's actually already a bit unfortunate that it's not done in the same VM in a background=true scenario but it shouldn't matter much I guess!)

----

PubapiVisitor.java

> List<String> api = new LinkedList<String>();

I suggest we use something better than a String here. The 'api' is a list of *what*?

----

> api.add(depth(indent) + "!TYPE " + e.getQualifiedName());

Are these lines that end up as-is in the sjavac_state file? I think we need better separation between domain objects and state file syntax. I suggest we gather all javac_state syntax related code (parsing and serialization) in one class. Preferrably BuildState.java.

(On my second read through, I realize that it might be better to address this in a separate cleanup. Fredrik, if you don't disagree profoundly with this suggestion, would you mind filing an issue on this?)

----

PubapiVisitor:

It looks like

> public void construct(TypeElement e)

constructs the 'api'. I'm surprised that it doesn't *return* the api. Why is it a 'void' method?

----

SourceLocation.java:

> } catch (FileNotFoundException e) {
>     System.err.println("Could not open "+sourceList.getPath()+" since it does not exist!");
>     return null;
> } catch (IOException e) {
>     System.err.println("Could not read "+sourceList.getPath());
>     return null;
> }

Is "return null" really appropriate here? How about returning Collections.emptySet or throwing an exception?

----

JavacServiceClient.java:

>  * Extract the package name and the class from the string.
>
> ...
>
>     int p = fullname.lastIndexOf('.');
>     if (p == -1) {
>         // Arghh, a source file without a package.
>         pkg = ":";
>     }
>     else {
>         pkg = ":"+fullname.substring(0, p);
>     }

The code is cluttered with this kind of string manipulation. I suggest we create a proper datatype instead of doing the above indexOf/substring thingy everywhere.

(Second read through: This could probably be addressed in a separate cleanup.)

----

JavacServiceClient.java:

This worries me:

> synchronized (deps) {
>     ...
> }

The only source of concurrency that I know of in sjavac is in PooledSjavac. And as long as an SjavacImpl object (which has no state) is passed as delegate to PooledSjavac, no races should be possible anywhere in the program.

If you agree, I suggest we drop 'synchronized' here, otherwise, please enlighten me on why it is neccessary.

----


/ Andreas