runtime changes for services

[Alan Bateman]

Thanks for going through this. I was slow getting back to this as I had 
to re-base the patch based on the latest updates and then I rang into a 
few javac issues. Comments inlined.

On 15/02/2012 23:04, Mandy Chung wrote:
> :
>
> On 2/9/2012 9:06 AM, Alan Bateman wrote:
>>   http://cr.openjdk.java.net/~alanb/jigsaw-services/webrev/index.html
> com/sun/classanalyzer/ModuleInfo.java
>   L46: you're right that it needs to make a defensive copy
>   of services and providers.
I added that comment as a reminder to check it. I've now changed it to 
make a copy.

>
>   L155-172: would it be better to sort the list before printing
>   the "requires services" statements?  I believe the ordering of
>   "provides service" statements matters and so it should keep
>   it the same order as specified in the META-INF/services files.
Yes, it should preserve the order. I've fixed this and the same thing in 
the jar tool where it was storing the provider names in a HashSet, thus 
loosing the original order.

>
> java/util/ServiceLoader.java
>   L357-359: for maintainability, would it be better to replace
>   these lines with
>      if (!hasNext()) {
>          throw new NoSuchElementException();
>      }
That would be clearer - thanks!

>
> org/openjdk/jigsaw/Context.java
>   L173, 203: I assume you want to fix these methods to return a
>   unmodifiable map, right?  I don't see the code that expects
>   them to be modifiable (maybe I miss something)?
This is just a comment as a reminder that the returned collection is 
mutable. All the current usages are read-only.


>
> org/openjdk/jigsaw/Linker.java
>   L296: extra blank line
OK
>
> org/openjdk/jigsaw/Loader.java
>   L286-289: can these lines be replaced with:
>   Set<String>  remotes = context.serviceSuppliers().get(cn);
You're right, this is clearer.

>
>   L278: I wonder if this should return a map of ModuleClassLoader
>   rather than ClassLoader just be explicit.
I don't have a strong opinion on this. I used the least specific type 
but we may need to change it to ModuleClassLoader in the future.

>
>   L291: this causes the module loader for the service
>   implementation class be instantiated. I presume we want
>   the loader be instantiated lazily when it's loaded. On
>   the other hand, the instantiation cost doesn't look like
>   too high.  It's fine for the initial implementation but
>   we should look into this open issue.
Okay, I'll note this as something to look at it again. The main thing is 
that the provider is instantiated lazily.

>
> org/openjdk/jigsaw/Resolver.java
>   L320,322,323, 338: a space below ":"
>   There are other places missing the space in the foreach
>   statements needed fixing too.
Our coding conventions badly need to be updated. Subjective as it is, I 
tend not to put a space before the colon.

>
> sun/tools/jar/Main.java
>   L785: would you suggest to replace Charset.forName("UTF-8")
>   with StandardCharsets.UTF_8 (no real difference though)?
There's a complicated story here. Charset.forName("UTF-8") is slightly 
faster at the moment.


>   L989: This is not really a question for this change but
>   you may have the answer. I have seen some copy of readAllBytes
>   in other places. Have we considered adding this support
>   to the platform?
We added Files.readAllBytes in JDK7 and I think someday we should do 
something for streams too.


>
> new services tests
>   Looks good.  The copyright start year should be 2012 :)
>   I notice that some tests turn on assertion but some don't.
>   It might be good to set -esa -ea on all tests I think.
>
At some point we need to pass the VM options through to the jmod/java 
commands (this goes generally for all the shell tests in the jdk repo). 
For now I'll use -esa consistently, there aren't any assertions in the 
java code in the tests.

I've put the updated webrev here:
http://cr.openjdk.java.net/~alanb/jigsaw-services/webrev.02/

-Alan.