Service configuration/context <was> Re: ServiceLoader.load* take 2

[Paul Sandoz]

Hi,

Here is a webrev of configuration changes for a more immutable approach to the contexts, just to show where this is heading:

  http://cr.openjdk.java.net/~psandoz/jigsaw/configuration/webrev/

The modular JDK builds OK and all jigsaw-related tests pass.

BaseContext, Context and PathContext are now interfaces. ContextBuilder and PathContextBuilder build up state to create immutable instances of Context and PathContext respectively. Linker also contains a builder LinkerContextBuilder that creates an immutable instance of Context.

ContextBuilder becomes ContextBuilderSetBuilder. There is no longer any need to check if the context instance is an instance of Context or LinkingContext, implementations of the method BaseContextBuilder.add(ModuleInfo mi) can encapsulate the different behavior. Thus LinkingContext is deleted.

The general steps are the same as before:

1) resolve

2) construct the context *builders*, using ContextBuilderSetBuilder

3, 4) link local and remote supplies, then build contexts from the context builders


Two awkward areas:

a) The method on PathContext:

      Set<PathContext> remoteContexts();

requires that a second pass be performed after construction of PathContext to make the instance immutable. I believe things could be simplified if that method was Set<String> remoteContexts() like on Context, then that method it could be moved to BaseContext. I left it alone for now since it may affect the java compiler (have not checked). 

b) Building contexts from context builders requires the map of context builder for module views is transformed into the corresponding map of context for module views. It's easy to do by maintaining a map of context for context builder, which anyway has to be done for a). However, it would be nice to remove this aspect if this information could be calculated by Configuration just from the contexts. I suspect it can given the way a stored configuration is loaded [*].


Next steps:

- Make Configuration immutable using a corresponding ConfigurationBuilder.

- Shift the state of services from Context to InstalledConfiguration (extends Configuration<Context>)

- Ensure that the set of contexts retains a topological order as traversed by the resolver. This will ensure service instances can be iterated over in a consistent order.

Paul.

[*] I notice that ContextBuilderSetBuilder is adding aliases to the map of context for module view:

  96     private void addContextForModuleView(CB cx, ModuleView mv) {
  97         contextForModuleView.put(mv.id().name(), cx);
  98         for (ModuleId alias : mv.aliases()) {
  99             contextForModuleView.put(alias.name(), cx);
 100         }
 101     }

but that information is never re-generated when a stored configuration is loaded in SimpleLibrary:

 598                 Context c = cx.build();                
 599                 assert c.name().equals(cxn);
 600                 cf.add(c);
 601                 
 602                 for (ModuleId m: c.modules()) {
 603                     for (ModuleId v: c.views(m)) {
 604                         cf.put(v.name(), c);
 605                     }
 606                 }

 Bug?


On Jun 20, 2012, at 3:06 PM, Paul Sandoz wrote:

> On Jun 12, 2012, at 5:03 PM, Paul Sandoz wrote:
>> - Modify the configuration so that service information is stored directly with the configuration rather than with each context (that is stored with the configuration). Currently service provider classes are looked up by iterating through all the contexts. This is not very efficient. The configuration is the place where to optimize.
>> This requires some refactoring of org.openjdk.jigsaw.Configuration because for instances of Configuration<PathContext> (the configuration produced when compiling) such state related to services is not relevant. This is also a potential opportunity to improve the immutability of Configuration using a builder pattern for construction.
> 
> 
> Some details on improving this area:
> 
> - Configuration has two constructors. One is used by the Linker/PathLinker to pass in all the configuration state. The other, that takes a subset of the complete state, is used by SimpleLibrary when parsing the serialized configuration and constructing the rest of the state while parsing. The methods Configuration.add/put are solely there for the purpose of SimpleLibrary to mutate state on Configuration. A Configuration builder could be used for both purposes.
> 
> - BaseContext.freeze. This transforms mutable state into immutable state, but only for state held by BaseContext and not for sub types (because it is not overridden). This is a sure sign that a builder pattern can be used to construct mutable state, then built into an immutable object.
> 
> - On Context there is code such as:
> 
>    public final Map<String,ModuleId> moduleForLocalClassMap() {
>        return Collections.unmodifiableMap(moduleForLocalClass);
>    }
> 
>  and see the more complex case for Context.services(), which will any way move to a subtype of Configuration.
> 
>  This class is really trying very hard to be immutable :-)
> 
> - In ContextBuilder there is casting, from line 132:
> 
>            if (cx instanceof LinkingContext) {
>                ((LinkingContext) cx).addModule(mi);
>            }
>            if (cx instanceof Context) {
>                URI lp = res.locationForName.get(mi.id().name());
>                if (lp != null) {
>                    String s = lp.getScheme();
>                    if (s == null || !s.equals("file")) {
>                        throw new AssertionError(s);
>                    }
>                    ((Context) cx).putLibraryPathForModule(mi.id(), new File(lp));
>                }
>            }
> 
>  this is a sign that the abstraction constructing a context is broken. Notice that a ContextFactory
>  instance is passed to the constructor of ContextBuilder. ContextBuilder is really a ContextSetBuilder, and
>  ContextFactory is acting a limited builder with just  the build() method implemented.
> 
> - Linker.LinkedContext is an internal detail thus could be a special builder that creates instances of Context. 
>  All the additional state held by Linker.LinkedContext is not relevant once the Configuration has been created,
>  as shown by the method on Linker to return the Configuration:
> 
>    static Configuration<org.openjdk.jigsaw.Context>
>        run(Library lib, ContextSet<Linker.Context> cxs)
> 
> - When i see LinkingContext i am inclined to think that there should be the following:
> 
>   interface Context // was class BaseContext
>   interface LinkingContext extends Context
>   abstract class AbstractContext implements Context
>   class InstallContext extends AbstractContext // was Context
>   class PathContext extends AbstractContext implements LinkingContext
>   class Linker.LinkerContext extends InstallContext implements LinkingContext // was Linker.Context
> 
>   class InstallConfiguration extends Configuration<InstallContext>
>     -- service state belongs here not on InstallContext
> 
> 
> Paul.