8154956: Module system implementation refresh (4/2016)

[Alan Bateman]

On 01/05/2016 23:27, Peter Levart wrote:
> Hi,
>
> I have some mostly stylish changes (which you can use or not) 
> regarding usage of Optional and Stream(s) in package java.lang.module:
>
> http://cr.openjdk.java.net/~plevart/jdk9-jake/ModuleSystem.referesh.201604/webrev.01/
It's good to do cleanup although our main focus here is to get through 
the transition to the new policy on root modules and the new -Xpatch as 
both are disruptive. We will continue to work in jake for a while as 
there are still many areas that need iteration and we need to go through 
a few rounds of API clean-up too.


> ...with some optimizations/cleanups/fixes:
>
> ModuleFinder.compose:
> - the composed ModuleFinder.findAll() is more efficient this way as it 
> doesn't need to call back to find().
True although if you don't mind, I'd prefer to skip this for because 
this method is due to be changed in a round of API updates (see 
JDK-8152650). The implementation will be replaced then.

>
> ModulePath:
> - there's unneeded overhead to call distinct() for a Stream that is 
> later collected into Collectors.toSet() as the collector already 
> uniquifies the elements.
> - lines 432...437 (old) - you don't want to treat entries in 
> SERVICES_PREFIX directory that end with ".class" as classFiles - so I 
> changes the:
>         .filter(e -> (e.endsWith(".class") || 
> e.startsWith(SERVICES_PREFIX)))
>   to:
>         .filter(s -> (s.endsWith(".class") ^ 
> s.startsWith(SERVICES_PREFIX)))
This looks good, I'll bring this in.

>
> ModuleReader:
> - the default method read(String name) should close the InputStream 
> after reading from it, shouldn't it?
Yes it should, just hasn't been noticed because each of the 
implementations implement this method. I don't know if you mean to put 
the try on the same line but I'll make that a bit more readable before 
pushing the patch.


>
> ModuleReferences:
> - no need for 'closed' flag in SafeCloseModuleReader to be volatile as 
> it is always accessed under lock (either read lock when reading or 
> write lock when writing).
You are right. There has been a few iterations on this code and I thin 
this was left over.

>
> Resolver:
> - Implemented caching of ResolvedModule(s) as they are entered into 
> resolved graph so that there are no duplicates.
> - Simplified last stage of iterative algorithm to build resolved 
> graph. There's no need to construct a map of changes and apply it 
> afterwards to 'g1' as changes can be applied individually for each 
> module 'm1' in the graph 'g1' as they are collected.
I would prefer not to change makeGraph in this round. You are right that 
it could be more efficient, Claes pointed this out recently too and had 
caching too. So something for a future round where it might be 
completely replaced.

-Alan