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

[Paul Sandoz]

Hi Mandy,

Here is the webrev for the context/configuration refactoring and services:

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

I chose to retain the existing approach to storing service information in the stored configuration. There is no point storing that information separately from the context itself, since it would just result in duplication of context names. The InstalledConfiguration class creates an optimized data structure for iterating globally over service provider class names for a given service interface.

I also took the opportunity to further simplify BaseContextBuilder and ContextBuilderSetBuilder:

- BaseContextBuilder does not implement equals/hashCode;

- ContextBuilderSetBuilder no longer needs to use an instance of IdentityHashSet to store context builder instances and rehash when creating the ContextBuilderSet; and

- I retained the freeze approach in BaseContextBuilder as it seems handy to catch potential bugs if the linking process attempts to monkey around with the set of modules IDs of a context builder.

There are potentially further improvements to better isolate the building of contexts by SimpleLibrary and Linker/PathLinker respectively, but i think at this point it is time to stop.

FWIW i understand the core Jigsaw code much better now :-)

Paul.

On Jun 28, 2012, at 4:47 PM, Paul Sandoz wrote:

> Hi Mandy,
> 
> Here is an updated webrev that also fixes aliases and cleans up the map of context for module view/alias: 
> 
>  http://cr.openjdk.java.net/~psandoz/jigsaw/configuration/webrev.1/
> 
> The configuration does not distinguish between a view and an alias (like ContextBuilderSetBuilder.contextForModuleView does not distinguish), so when you dump the config it will be listed as a view , i dunno if this a big deal, it's easy to fix it so there is a method BaseContext.aliases(ModuleId mid), and aliases are stored separately to views.
> 
> Paul.
> 
> 
> On Jun 28, 2012, at 2:46 PM, Paul Sandoz wrote:
> 
>> On Jun 28, 2012, at 9:37 AM, Mandy Chung wrote:
>>> On 6/27/2012 7:52 AM, Paul Sandoz wrote:
>>>> [*] 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?
>>> 
>>> 
>>> In fact, the aliases information are not stored in the configuration.  It's a bug - good catch.  Currently the jdk implementation doesn't depend on it.  This only affects ModuleClassLoader.isModulePresent("java.xml") if the parameter is an alias name.
>>> 
>> 
>> OK, i just fixed it to support aliases, the simplest thing to do is ensure that aliases+views are returned by BaseContext.views(ModuleId ), thus they become stored as part of the configuration. I now see stuff like "java.<x>" in the dump config.
>> 
>> I think this sort of confirms we can simplify Configuration to always calculate the map of context for module view on construction in one place. Thus there is no need to have explicit methods to update that map state or pass in that map state on construction and more importantly there is no need to trust that the map state is in sync with the contexts themselves. It will also simplify Context*Set and the process of building Context instances.
>> 
>> 
>>> Mandy
>>> P.S. I'll try to look at your webrev and understand your proposed approach next week.
>> 
>> OK thanks, i expect by then i will have updated to also include the 2 next steps in the previous email.
>> 
>> I am a bit nervous about size of the changes, it's tricky to unravel this piece by piece.
>> 
>> Paul.
>