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

[Paul Sandoz]

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.