Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)

Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)
On 7/1/2012 2:34 AM, Paul Sandoz wrote:
> Hi,
>
> I thought it would help to consolidate the service/configuration changes into one email.

Yes, that helps. Thanks.

> Service provider module resolution
> -----------------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>
> - "permits" clauses do not affect service provider module resolution
>
> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>
> I understand that on and off list there is reasonable agreement to go forward with this latter approach.

Doing the service provider dependency resolution in phases would make it
a little easier to trace and diagnose. Each phase may add new modules
to the graph (service provider module and its dependencies) and looks
like it requires to re-validate if a module is required optionally but
failed to be chosen in phase 0. For example,

M
requires optional X;
requires service S;

P
requires X @ 1.0
provides service S with S1;

X @ 1.0
permits P

In this case, phase 0 will chose M only. Phase 1 choses P and and
the solution includes M, P, and . However, 't permit M
but it's linked with M in your current implementation.
Nits w.r.t. the change:
Resolver.javaL221: toString() call is not needed
L446: best to follow current convention to put the first parameter
next to the signature L445 and line break at the second parameter.
L450: 4-space indent would be good.
L458: typo 'dependencey'
L533: if serviceInterface was resolved in previous phases, it can skip
and process the next service interface.

many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
and permits is ignored.

I think it would be useful to add a few test cases that service provider
module pulls in new modules and how this fix changes the solution - e.g.
a case when a provider module is not included due to a module selected
in phase 0 doesn't satisfy its dependency.

Mandy

)

Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)
On 7/1/2012 2:34 AM, Paul Sandoz wrote:
> Hi,
>
> I thought it would help to consolidate the service/configuration changes into one email.

Yes, that helps. Thanks.

> Service provider module resolution
> -----------------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>
> - "permits" clauses do not affect service provider module resolution
>
> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>
> I understand that on and off list there is reasonable agreement to go forward with this latter approach.

Doing the service provider dependency resolution in phases would make it
a little easier to trace and diagnose. Each phase may add new modules
to the graph (service provider module and its dependencies) and looks
like it requires to re-validate if a module is required optionally but
failed to be chosen in phase 0. For example,

M
requires optional X;
requires service S;

P
requires X @ 1.0
provides service S with S1;

X @ 1.0
permits P

In this case, phase 0 will chose M only. Phase 1 choses P and and
the solution includes M, P, and . However, 't permit M
but it's linked with M in your current implementation.
Nits w.r.t. the change:
Resolver.javaL221: toString() call is not needed
L446: best to follow current convention to put the first parameter
next to the signature L445 and line break at the second parameter.
L450: 4-space indent would be good.
L458: typo 'dependencey'
L533: if serviceInterface was resolved in previous phases, it can skip
and process the next service interface.

many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
and permits is ignored.

I think it would be useful to add a few test cases that service provider
module pulls in new modules and how this fix changes the solution - e.g.
a case when a provider module is not included due to a module selected
in phase 0 doesn't satisfy its dependency.

Mandy

)


On 7/5/2012 2:47 AM, Paul Sandoz wrote:
>
>> and looks like it requires to re-validate if a module is required optionally but failed to be chosen in phase 0.
>> For example,
>>
>> M
>> requires optional X;
>> requires service S;
>>
>> P
>> requires X @ 1.0
>> provides service S with S1;
>>
>> X @ 1.0
>> permits P
>>
>> In this case, phase 0 will chose M only. Phase 1 choses P and the solution includes M, P, . However,'t permit M but it's linked with M in your current implementation.
> Well spotted. Hmm... need to put my thinking cap on and investigate!
>
> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>
> {
> permits y;
> }
>
> {
> requires x;
> }
>
> {
> requires optional x;
> }
>
> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.

The attachment is missing. The resulting solution would be only
which is correct. In the example I showed above, the service provider
module P requires a module that doesn't satisfy the constraints in
the previously resolved modules. I would consider that module P will
not be included as its dependency fails to be resolved.

> So i think we have a general bug and require either:
>
> 1) a validation step on the set of resolved modules; or
>
> 2) the compile and install time linkers do the right thing.
>
> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.

In your example, there is no service provider module in the picture.
The current implementation should resolve z with no x module in the
solution. Just to make sure this is the behavior you're thinking about
- if x is required by a service provider module p being resolved in
phase 1 due to (z but not x is one of the resolved module in phase 0),
would the service provider module requiring x be included in the solution?
>> L446: best to follow current convention to put the first parameter
>> next to the signature L445 and line break at the second parameter.
>> L450: 4-space indent would be good.
> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!

One inconsistency I know of is the for-each statement "for (T a:
iterable)" vs "for (T a : iterable)" whether a space is needed after
":". Other than that the coding convention for jigsaw is fairly
consistently applied in the org.openjdk.jigsaw source e.g. 4-space
indentation, and line breaks. Is there any other inconsistency you observe?

> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>
>
>> L458: typo 'dependencey'
>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>
> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.

Is it possible to have a service provider dependency that failed to
resolve in phase n but successfully resolved later in phase > n? During
resolution, the module libraries should be locked and there should be no
change to the list of available modules. In any case, I think it's
fine not to check that and the resolver/library would do better caching
and avoid reading the same set of module-info multiple times.

>
>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>> and permits is ignored.
>>
>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>
> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>
> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.

I see and you have several changes lined up. Would you consider to
include the tests update in part of the refactoring webrev? Test cases
are sometimes helpful for code review.

Mandy
)

Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)
On 7/1/2012 2:34 AM, Paul Sandoz wrote:
> Hi,
>
> I thought it would help to consolidate the service/configuration changes into one email.

Yes, that helps. Thanks.

> Service provider module resolution
> -----------------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>
> - "permits" clauses do not affect service provider module resolution
>
> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>
> I understand that on and off list there is reasonable agreement to go forward with this latter approach.

Doing the service provider dependency resolution in phases would make it
a little easier to trace and diagnose. Each phase may add new modules
to the graph (service provider module and its dependencies) and looks
like it requires to re-validate if a module is required optionally but
failed to be chosen in phase 0. For example,

M
requires optional X;
requires service S;

P
requires X @ 1.0
provides service S with S1;

X @ 1.0
permits P

In this case, phase 0 will chose M only. Phase 1 choses P and and
the solution includes M, P, and . However, 't permit M
but it's linked with M in your current implementation.
Nits w.r.t. the change:
Resolver.javaL221: toString() call is not needed
L446: best to follow current convention to put the first parameter
next to the signature L445 and line break at the second parameter.
L450: 4-space indent would be good.
L458: typo 'dependencey'
L533: if serviceInterface was resolved in previous phases, it can skip
and process the next service interface.

many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
and permits is ignored.

I think it would be useful to add a few test cases that service provider
module pulls in new modules and how this fix changes the solution - e.g.
a case when a provider module is not included due to a module selected
in phase 0 doesn't satisfy its dependency.

Mandy

)


On 7/5/2012 2:47 AM, Paul Sandoz wrote:
>
>> and looks like it requires to re-validate if a module is required optionally but failed to be chosen in phase 0.
>> For example,
>>
>> M
>> requires optional X;
>> requires service S;
>>
>> P
>> requires X @ 1.0
>> provides service S with S1;
>>
>> X @ 1.0
>> permits P
>>
>> In this case, phase 0 will chose M only. Phase 1 choses P and the solution includes M, P, . However,'t permit M but it's linked with M in your current implementation.
> Well spotted. Hmm... need to put my thinking cap on and investigate!
>
> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>
> {
> permits y;
> }
>
> {
> requires x;
> }
>
> {
> requires optional x;
> }
>
> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.

The attachment is missing. The resulting solution would be only
which is correct. In the example I showed above, the service provider
module P requires a module that doesn't satisfy the constraints in
the previously resolved modules. I would consider that module P will
not be included as its dependency fails to be resolved.

> So i think we have a general bug and require either:
>
> 1) a validation step on the set of resolved modules; or
>
> 2) the compile and install time linkers do the right thing.
>
> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.

In your example, there is no service provider module in the picture.
The current implementation should resolve z with no x module in the
solution. Just to make sure this is the behavior you're thinking about
- if x is required by a service provider module p being resolved in
phase 1 due to (z but not x is one of the resolved module in phase 0),
would the service provider module requiring x be included in the solution?
>> L446: best to follow current convention to put the first parameter
>> next to the signature L445 and line break at the second parameter.
>> L450: 4-space indent would be good.
> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!

One inconsistency I know of is the for-each statement "for (T a:
iterable)" vs "for (T a : iterable)" whether a space is needed after
":". Other than that the coding convention for jigsaw is fairly
consistently applied in the org.openjdk.jigsaw source e.g. 4-space
indentation, and line breaks. Is there any other inconsistency you observe?

> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>
>
>> L458: typo 'dependencey'
>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>
> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.

Is it possible to have a service provider dependency that failed to
resolve in phase n but successfully resolved later in phase > n? During
resolution, the module libraries should be locked and there should be no
change to the list of available modules. In any case, I think it's
fine not to check that and the resolver/library would do better caching
and avoid reading the same set of module-info multiple times.

>
>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>> and permits is ignored.
>>
>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>
> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>
> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.

I see and you have several changes lined up. Would you consider to
include the tests update in part of the refactoring webrev? Test cases
are sometimes helpful for code review.

Mandy
)
On Jul 5, 2012, at 5:13 PM, Mandy Chung wrote:
>>>
>> Well spotted. Hmm... need to put my thinking cap on and investigate!
>>
>> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>>
>> {
>> permits y;
>> }
>>
>> {
>> requires x;
>> }
>>
>> {
>> requires optional x;
>> }
>>
>> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.
>
> The attachment is missing.

That attachment got stripped by the mail server :-( i will resend to you privately.


> The resulting solution would be only which is correct.

No, because module y is a required root module; x y and z will be compiled at the same time by javac (would be clearer if the attachment was present!).

For me the solution is:

[exec] | Configured for [, , ]

If I rename module z to y and module y to z the resolution will fail:

[exec] error: Cannot resolve module dependencies using Jigsaw module resolver
[exec] [x@=1.0, y@=1.0, z@=1.0]: Cannot resolve


> In the example I showed above, the service provider module P requires a module that doesn't satisfy the constraints in the previously resolved modules.

I think the issue is more general. A service provider dependency is equivalent to an optional root dependency.


> I would consider that module P will not be included as its dependency fails to be resolved.
>

OK.


>> So i think we have a general bug and require either:
>>
>> 1) a validation step on the set of resolved modules; or
>>
>> 2) the compile and install time linkers do the right thing.
>>
>> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.
>
> In your example, there is no service provider module in the picture. The current implementation should resolve z with no x module in the solution. Just to make sure this is the behavior you're thinking about - if x is required by a service provider module p being resolved in phase 1 due to (z but not x is one of the resolved module in phase 0), would the service provider module requiring x be included in the solution?

Yes i am beginning to think so, x would be included in the configuration but z would not be linked to x. To me that makes sense because of the "requires optional x" in z.

Why should, for a failed resolution of x in z, the optionality of x in z affect other modules that can resolve a dependency on x?

I realize that option 1) above does not make sense given the optionality.


>>> L446: best to follow current convention to put the first parameter
>>> next to the signature L445 and line break at the second parameter.
>>> L450: 4-space indent would be good.
>> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!
>
> One inconsistency I know of is the for-each statement "for (T a: iterable)" vs "for (T a : iterable)" whether a space is needed after ":". Other than that the coding convention for jigsaw is fairly consistently applied in the org.openjdk.jigsaw source e.g. 4-space indentation, and line breaks. Is there any other inconsistency you observe?
>

It is not so much the existing inconsistency, i don't know to what extent the code is or is not consistent, but the effort required to not be inconsistent and the effort required to review code and point out the inconsistencies. It's something a tool like NBs can do. We just need agreement on the style and i can come up with a NBs formatting configuration.


>> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>>
>>
>>> L458: typo 'dependencey'
>>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>>
>> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.
>
> Is it possible to have a service provider dependency that failed to resolve in phase n but successfully resolved later in phase > n? During resolution, the module libraries should be locked and there should be no change to the list of available modules.

Good point, i am struggling to come up with an example.


> In any case, I think it's fine not to check that and the resolver/library would do better caching and avoid reading the same set of module-info multiple times.

>
>>
>>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>>> and permits is ignored.
>>>
>>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
>> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>>
>> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>>
>> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.
>
> I see and you have several changes lined up. Would you consider to include the tests update in part of the refactoring webrev? Test cases are sometimes helpful for code review.
>

Yes. To keep things easier for everyone else it is probably best if keep work on the tip and pull it up stream as required.

Paul.)

Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)
On 7/1/2012 2:34 AM, Paul Sandoz wrote:
> Hi,
>
> I thought it would help to consolidate the service/configuration changes into one email.

Yes, that helps. Thanks.

> Service provider module resolution
> -----------------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>
> - "permits" clauses do not affect service provider module resolution
>
> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>
> I understand that on and off list there is reasonable agreement to go forward with this latter approach.

Doing the service provider dependency resolution in phases would make it
a little easier to trace and diagnose. Each phase may add new modules
to the graph (service provider module and its dependencies) and looks
like it requires to re-validate if a module is required optionally but
failed to be chosen in phase 0. For example,

M
requires optional X;
requires service S;

P
requires X @ 1.0
provides service S with S1;

X @ 1.0
permits P

In this case, phase 0 will chose M only. Phase 1 choses P and and
the solution includes M, P, and . However, 't permit M
but it's linked with M in your current implementation.
Nits w.r.t. the change:
Resolver.javaL221: toString() call is not needed
L446: best to follow current convention to put the first parameter
next to the signature L445 and line break at the second parameter.
L450: 4-space indent would be good.
L458: typo 'dependencey'
L533: if serviceInterface was resolved in previous phases, it can skip
and process the next service interface.

many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
and permits is ignored.

I think it would be useful to add a few test cases that service provider
module pulls in new modules and how this fix changes the solution - e.g.
a case when a provider module is not included due to a module selected
in phase 0 doesn't satisfy its dependency.

Mandy

)


On 7/5/2012 2:47 AM, Paul Sandoz wrote:
>
>> and looks like it requires to re-validate if a module is required optionally but failed to be chosen in phase 0.
>> For example,
>>
>> M
>> requires optional X;
>> requires service S;
>>
>> P
>> requires X @ 1.0
>> provides service S with S1;
>>
>> X @ 1.0
>> permits P
>>
>> In this case, phase 0 will chose M only. Phase 1 choses P and the solution includes M, P, . However,'t permit M but it's linked with M in your current implementation.
> Well spotted. Hmm... need to put my thinking cap on and investigate!
>
> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>
> {
> permits y;
> }
>
> {
> requires x;
> }
>
> {
> requires optional x;
> }
>
> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.

The attachment is missing. The resulting solution would be only
which is correct. In the example I showed above, the service provider
module P requires a module that doesn't satisfy the constraints in
the previously resolved modules. I would consider that module P will
not be included as its dependency fails to be resolved.

> So i think we have a general bug and require either:
>
> 1) a validation step on the set of resolved modules; or
>
> 2) the compile and install time linkers do the right thing.
>
> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.

In your example, there is no service provider module in the picture.
The current implementation should resolve z with no x module in the
solution. Just to make sure this is the behavior you're thinking about
- if x is required by a service provider module p being resolved in
phase 1 due to (z but not x is one of the resolved module in phase 0),
would the service provider module requiring x be included in the solution?
>> L446: best to follow current convention to put the first parameter
>> next to the signature L445 and line break at the second parameter.
>> L450: 4-space indent would be good.
> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!

One inconsistency I know of is the for-each statement "for (T a:
iterable)" vs "for (T a : iterable)" whether a space is needed after
":". Other than that the coding convention for jigsaw is fairly
consistently applied in the org.openjdk.jigsaw source e.g. 4-space
indentation, and line breaks. Is there any other inconsistency you observe?

> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>
>
>> L458: typo 'dependencey'
>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>
> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.

Is it possible to have a service provider dependency that failed to
resolve in phase n but successfully resolved later in phase > n? During
resolution, the module libraries should be locked and there should be no
change to the list of available modules. In any case, I think it's
fine not to check that and the resolver/library would do better caching
and avoid reading the same set of module-info multiple times.

>
>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>> and permits is ignored.
>>
>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>
> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>
> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.

I see and you have several changes lined up. Would you consider to
include the tests update in part of the refactoring webrev? Test cases
are sometimes helpful for code review.

Mandy
)
On Jul 5, 2012, at 5:13 PM, Mandy Chung wrote:
>>>
>> Well spotted. Hmm... need to put my thinking cap on and investigate!
>>
>> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>>
>> {
>> permits y;
>> }
>>
>> {
>> requires x;
>> }
>>
>> {
>> requires optional x;
>> }
>>
>> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.
>
> The attachment is missing.

That attachment got stripped by the mail server :-( i will resend to you privately.


> The resulting solution would be only which is correct.

No, because module y is a required root module; x y and z will be compiled at the same time by javac (would be clearer if the attachment was present!).

For me the solution is:

[exec] | Configured for [, , ]

If I rename module z to y and module y to z the resolution will fail:

[exec] error: Cannot resolve module dependencies using Jigsaw module resolver
[exec] [x@=1.0, y@=1.0, z@=1.0]: Cannot resolve


> In the example I showed above, the service provider module P requires a module that doesn't satisfy the constraints in the previously resolved modules.

I think the issue is more general. A service provider dependency is equivalent to an optional root dependency.


> I would consider that module P will not be included as its dependency fails to be resolved.
>

OK.


>> So i think we have a general bug and require either:
>>
>> 1) a validation step on the set of resolved modules; or
>>
>> 2) the compile and install time linkers do the right thing.
>>
>> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.
>
> In your example, there is no service provider module in the picture. The current implementation should resolve z with no x module in the solution. Just to make sure this is the behavior you're thinking about - if x is required by a service provider module p being resolved in phase 1 due to (z but not x is one of the resolved module in phase 0), would the service provider module requiring x be included in the solution?

Yes i am beginning to think so, x would be included in the configuration but z would not be linked to x. To me that makes sense because of the "requires optional x" in z.

Why should, for a failed resolution of x in z, the optionality of x in z affect other modules that can resolve a dependency on x?

I realize that option 1) above does not make sense given the optionality.


>>> L446: best to follow current convention to put the first parameter
>>> next to the signature L445 and line break at the second parameter.
>>> L450: 4-space indent would be good.
>> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!
>
> One inconsistency I know of is the for-each statement "for (T a: iterable)" vs "for (T a : iterable)" whether a space is needed after ":". Other than that the coding convention for jigsaw is fairly consistently applied in the org.openjdk.jigsaw source e.g. 4-space indentation, and line breaks. Is there any other inconsistency you observe?
>

It is not so much the existing inconsistency, i don't know to what extent the code is or is not consistent, but the effort required to not be inconsistent and the effort required to review code and point out the inconsistencies. It's something a tool like NBs can do. We just need agreement on the style and i can come up with a NBs formatting configuration.


>> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>>
>>
>>> L458: typo 'dependencey'
>>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>>
>> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.
>
> Is it possible to have a service provider dependency that failed to resolve in phase n but successfully resolved later in phase > n? During resolution, the module libraries should be locked and there should be no change to the list of available modules.

Good point, i am struggling to come up with an example.


> In any case, I think it's fine not to check that and the resolver/library would do better caching and avoid reading the same set of module-info multiple times.

>
>>
>>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>>> and permits is ignored.
>>>
>>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
>> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>>
>> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>>
>> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.
>
> I see and you have several changes lined up. Would you consider to include the tests update in part of the refactoring webrev? Test cases are sometimes helpful for code review.
>

Yes. To keep things easier for everyone else it is probably best if keep work on the tip and pull it up stream as required.

Paul.)
Paul,

On 7/1/2012 2:34 AM, Paul Sandoz wrote:

> Service state on configuration
> ----------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

Sorry for the delay on getting to this one.

This change keeps a global map of service interface to its service
provider class names in the configuration for a more efficient service
lookup and service instance creation at runtime which is good.

On the other hand, I'm not sure about the new InstalledConfiguration
class, an extension to Configuration. While service lookup is only
needed at runtime, it seems to me that Configuration would
be the subclass of Configuration rather than Configuration.
With modulepath, there isn't any installed configuration that some
refactoring might be needed. There will be some API refactoring/cleanup
anyway (as you have already attempted). I wonder if this patch can simply
define the Configuration.findServices method that might well be an
abstract method implemented by the InstalledConfiguration class
which will only be an implementation class used by StoredConfiguration.

Mandy

)

Hi,

I thought it would help to consolidate the service/configuration changes into one email. Each is like a plane circling in a holding pattern waiting to land at Heathrow...

I have also separated out the configuration/context refactoring into a separate webrev.


Service provider module resolution
-----------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/

- "permits" clauses do not affect service provider module resolution

- service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.

I understand that on and off list there is reasonable agreement to go forward with this latter approach.


Service state on configuration
----------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

An extension of Configuration called InstalledConfiguration that encapsulates state mapping service interface class names to context and service provider class names for more optimal service instance creation. This state is exposed as an Iterable of a tuple of Context and service provider class name. An Iterable, rather than a Map, assumes much less on the underlying state representation.

Context and persistence of service information is left unmodified. IMHO there is little benefit in separating this out. In terms of storage the service provider class names need to be associated with the correspond context. I felt it best to avoid duplication of context names in the persisted storage.

Note that Configuration already performs another optimization, creating the map of context for context name, and the refactoring changes (see below) create the map of context for module view/alias name.


Refactoring of configuration and context
------------------------------------------------------

http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/webrev/

This is my attempt to refactor (or "curate" :-) ) the configuration and context classes to make "cleaner" the approach of building state of contexts and configurations then creating instances of contexts and configurations.

I have tried to reduce the assumptions between things in the process of resolving, context builder construction (grouping modules into contexts), and linking local and remote suppliers.

IMHO we need to gradually transition this code base from it's prototype-like state; much like Chris is doing for module file parsing/reading i am attempting to do for another area.


http://cr.openjdk.java.net/~psandoz/jigsaw/config-refactor/config-services-webrev/

The above webrev is the same as "Service state on configuration" functionality but rebased on the refactoring changes (Mercurial queues are useful!). You may observe that it integrates more cleanly with SimpleLibrary when building the installed configuration from storage as there is no need for an explicit "initialization" step to calculate the optimal service look up state.


Next steps
--------------

- The test class _Configuration (and associated test builder classes) need to updated to support the building of contexts with service information and new tests need to be added verify service provider module dependency resolution. This may be an opportunity to see if TestNG can be used?

- Retain topological order (from resolver's depth first search) for modules, contexts and service state. Iteration order of entries for HashMap and HashSet are not guaranteed if even it has up to now been mostly stable. The hash code related fixes in JDK8 will change that. Thus we need to explicitly support a consistent ordering using LinkedHashMap and LinkedHashSet. This is also the point at which to optimize the copying of state when building and constructing the configuration and contexts (i want persistent collections like those in Clojure and Scala!).

Paul.)
On 7/1/2012 2:34 AM, Paul Sandoz wrote:
> Hi,
>
> I thought it would help to consolidate the service/configuration changes into one email.

Yes, that helps. Thanks.

> Service provider module resolution
> -----------------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/resolver-services/webrev.2/
>
> - "permits" clauses do not affect service provider module resolution
>
> - service provider module resolution occurs after "application" resolution. Essentially the application has to successfully resolve as if there were no "requires service " clauses present and after that service provider module resolution occurs.
>
> I understand that on and off list there is reasonable agreement to go forward with this latter approach.

Doing the service provider dependency resolution in phases would make it
a little easier to trace and diagnose. Each phase may add new modules
to the graph (service provider module and its dependencies) and looks
like it requires to re-validate if a module is required optionally but
failed to be chosen in phase 0. For example,

M
requires optional X;
requires service S;

P
requires X @ 1.0
provides service S with S1;

X @ 1.0
permits P

In this case, phase 0 will chose M only. Phase 1 choses P and and
the solution includes M, P, and . However, 't permit M
but it's linked with M in your current implementation.
Nits w.r.t. the change:
Resolver.javaL221: toString() call is not needed
L446: best to follow current convention to put the first parameter
next to the signature L445 and line break at the second parameter.
L450: 4-space indent would be good.
L458: typo 'dependencey'
L533: if serviceInterface was resolved in previous phases, it can skip
and process the next service interface.

many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
and permits is ignored.

I think it would be useful to add a few test cases that service provider
module pulls in new modules and how this fix changes the solution - e.g.
a case when a provider module is not included due to a module selected
in phase 0 doesn't satisfy its dependency.

Mandy

)


On 7/5/2012 2:47 AM, Paul Sandoz wrote:
>
>> and looks like it requires to re-validate if a module is required optionally but failed to be chosen in phase 0.
>> For example,
>>
>> M
>> requires optional X;
>> requires service S;
>>
>> P
>> requires X @ 1.0
>> provides service S with S1;
>>
>> X @ 1.0
>> permits P
>>
>> In this case, phase 0 will chose M only. Phase 1 choses P and the solution includes M, P, . However,'t permit M but it's linked with M in your current implementation.
> Well spotted. Hmm... need to put my thinking cap on and investigate!
>
> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>
> {
> permits y;
> }
>
> {
> requires x;
> }
>
> {
> requires optional x;
> }
>
> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.

The attachment is missing. The resulting solution would be only
which is correct. In the example I showed above, the service provider
module P requires a module that doesn't satisfy the constraints in
the previously resolved modules. I would consider that module P will
not be included as its dependency fails to be resolved.

> So i think we have a general bug and require either:
>
> 1) a validation step on the set of resolved modules; or
>
> 2) the compile and install time linkers do the right thing.
>
> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.

In your example, there is no service provider module in the picture.
The current implementation should resolve z with no x module in the
solution. Just to make sure this is the behavior you're thinking about
- if x is required by a service provider module p being resolved in
phase 1 due to (z but not x is one of the resolved module in phase 0),
would the service provider module requiring x be included in the solution?
>> L446: best to follow current convention to put the first parameter
>> next to the signature L445 and line break at the second parameter.
>> L450: 4-space indent would be good.
> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!

One inconsistency I know of is the for-each statement "for (T a:
iterable)" vs "for (T a : iterable)" whether a space is needed after
":". Other than that the coding convention for jigsaw is fairly
consistently applied in the org.openjdk.jigsaw source e.g. 4-space
indentation, and line breaks. Is there any other inconsistency you observe?

> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>
>
>> L458: typo 'dependencey'
>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>
> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.

Is it possible to have a service provider dependency that failed to
resolve in phase n but successfully resolved later in phase > n? During
resolution, the module libraries should be locked and there should be no
change to the list of available modules. In any case, I think it's
fine not to check that and the resolver/library would do better caching
and avoid reading the same set of module-info multiple times.

>
>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>> and permits is ignored.
>>
>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>
> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>
> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.

I see and you have several changes lined up. Would you consider to
include the tests update in part of the refactoring webrev? Test cases
are sometimes helpful for code review.

Mandy
)
On Jul 5, 2012, at 5:13 PM, Mandy Chung wrote:
>>>
>> Well spotted. Hmm... need to put my thinking cap on and investigate!
>>
>> A service provider module choice from the perspective of the resolver is the same as a root module choice. I don't currently see how the above case is different from:
>>
>> {
>> permits y;
>> }
>>
>> {
>> requires x;
>> }
>>
>> {
>> requires optional x;
>> }
>>
>> when the order of resolution is z, y, then x. I have attached a simple test case that resolves using the above modules.
>
> The attachment is missing.

That attachment got stripped by the mail server :-( i will resend to you privately.


> The resulting solution would be only which is correct.

No, because module y is a required root module; x y and z will be compiled at the same time by javac (would be clearer if the attachment was present!).

For me the solution is:

[exec] | Configured for [, , ]

If I rename module z to y and module y to z the resolution will fail:

[exec] error: Cannot resolve module dependencies using Jigsaw module resolver
[exec] [x@=1.0, y@=1.0, z@=1.0]: Cannot resolve


> In the example I showed above, the service provider module P requires a module that doesn't satisfy the constraints in the previously resolved modules.

I think the issue is more general. A service provider dependency is equivalent to an optional root dependency.


> I would consider that module P will not be included as its dependency fails to be resolved.
>

OK.


>> So i think we have a general bug and require either:
>>
>> 1) a validation step on the set of resolved modules; or
>>
>> 2) the compile and install time linkers do the right thing.
>>
>> Which one we choose depends on whether we consider the above a valid solution or not. Can x be present but the linker knows that z should not link to x? i.e. as if the "resolve optional x" gets erased by the resolver by adapting the module info of z. It seems reasonable.
>
> In your example, there is no service provider module in the picture. The current implementation should resolve z with no x module in the solution. Just to make sure this is the behavior you're thinking about - if x is required by a service provider module p being resolved in phase 1 due to (z but not x is one of the resolved module in phase 0), would the service provider module requiring x be included in the solution?

Yes i am beginning to think so, x would be included in the configuration but z would not be linked to x. To me that makes sense because of the "requires optional x" in z.

Why should, for a failed resolution of x in z, the optionality of x in z affect other modules that can resolve a dependency on x?

I realize that option 1) above does not make sense given the optionality.


>>> L446: best to follow current convention to put the first parameter
>>> next to the signature L445 and line break at the second parameter.
>>> L450: 4-space indent would be good.
>> Can we reach agreement on a code convention for Jigsaw that is supported by the NetBeans formatting options? then i can just hit Ctrl+Shift+F :-) i fully admit i am lazy!
>
> One inconsistency I know of is the for-each statement "for (T a: iterable)" vs "for (T a : iterable)" whether a space is needed after ":". Other than that the coding convention for jigsaw is fairly consistently applied in the org.openjdk.jigsaw source e.g. 4-space indentation, and line breaks. Is there any other inconsistency you observe?
>

It is not so much the existing inconsistency, i don't know to what extent the code is or is not consistent, but the effort required to not be inconsistent and the effort required to review code and point out the inconsistencies. It's something a tool like NBs can do. We just need agreement on the style and i can come up with a NBs formatting configuration.


>> I don't have any strong preference on the style, I just want to be consistent and do it quickly to avoid us spending unnecessary cycles on these aspects.
>>
>>
>>> L458: typo 'dependencey'
>>> L533: if serviceInterface was resolved in previous phases, it can skip and process the next service interface.
>>>
>> I was wondering about this. A service provider dependency that failed to resolve in phase n might be resolved later in a phase> n. So i left that as is for now. Also I did not bother to check if a service provider dependency was already resolved since it is done in the resolve method and i wanted the tracing to show what was going on.
>
> Is it possible to have a service provider dependency that failed to resolve in phase n but successfully resolved later in phase > n? During resolution, the module libraries should be locked and there should be no change to the list of available modules.

Good point, i am struggling to come up with an example.


> In any case, I think it's fine not to check that and the resolver/library would do better caching and avoid reading the same set of module-info multiple times.

>
>>
>>> many.sh L119 - it'd be good to add a comment that p4 is 1 implementation
>>> and permits is ignored.
>>>
>>> I think it would be useful to add a few test cases that service provider module pulls in new modules and how this fix changes the solution - e.g. a case when a provider module is not included due to a module selected in phase 0 doesn't satisfy its dependency.
>> Yes, i want to flesh out the _Configurator related tests to support services, that should make redundant some of the shell-script-based tests, those tests can then focus on the use of ServiceLoader and we can probably consolidate.
>>
>> I have a preference to do that in another webrev, since it is gonna result in some shuffling around is that OK?
>>
>> Also i have a preference to do that after the refactoring changes (which somewhat cleans up that test code), but realize that could hold things up.
>
> I see and you have several changes lined up. Would you consider to include the tests update in part of the refactoring webrev? Test cases are sometimes helpful for code review.
>

Yes. To keep things easier for everyone else it is probably best if keep work on the tip and pull it up stream as required.

Paul.)
Paul,

On 7/1/2012 2:34 AM, Paul Sandoz wrote:

> Service state on configuration
> ----------------------------------------
>
> http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/

Sorry for the delay on getting to this one.

This change keeps a global map of service interface to its service
provider class names in the configuration for a more efficient service
lookup and service instance creation at runtime which is good.

On the other hand, I'm not sure about the new InstalledConfiguration
class, an extension to Configuration. While service lookup is only
needed at runtime, it seems to me that Configuration would
be the subclass of Configuration rather than Configuration.
With modulepath, there isn't any installed configuration that some
refactoring might be needed. There will be some API refactoring/cleanup
anyway (as you have already attempted). I wonder if this patch can simply
define the Configuration.findServices method that might well be an
abstract method implemented by the InstalledConfiguration class
which will only be an implementation class used by StoredConfiguration.

Mandy

)
Hi Mandy,

On Aug 8, 2012, at 2:55 AM, Mandy Chung <> wrote:

> Paul,
>
> On 7/1/2012 2:34 AM, Paul Sandoz wrote:
>
>> Service state on configuration
>> ----------------------------------------
>>
>> http://cr.openjdk.java.net/~psandoz/jigsaw/config-services/webrev/
>
> Sorry for the delay on getting to this one.
>
> This change keeps a global map of service interface to its service
> provider class names in the configuration for a more efficient service
> lookup and service instance creation at runtime which is good.
>

Yes, plus it does not expose the Map, it only exposes the look up using Iterable given a service interface class, thus allowing more flexibility of the implementation.


> On the other hand, I'm not sure about the new InstalledConfiguration
> class, an extension to Configuration. While service lookup is only
> needed at runtime, it seems to me that Configuration would
> be the subclass of Configuration rather than Configuration.

There is currently no additional functionality specific to Configuration that a subtype is required for that.


> With modulepath, there isn't any installed configuration that some
> refactoring might be needed.

Are you referring to the potential execution from compiled exploded modules?

If so, there are still services present, so it will still likely be the same as Configuration and using org.openjdk.jigsaw.Linker, where the storage is abstracted using a special Library implementation to handle the exploded layout of modules and class files [*]. AND, it does not negate caching/storing the configuration on disk if we know that nothing has changed for a subsequent execution.

If not, InstalledConfiguration is never referenced for compile-time related functionality. PathContext holds no service-related information and thus PathLinker does not manipulate such information.

--

Note that potentially we may in the future control the iteration order of service provider classes i.e. the order which they are stored in the configuration, so the InstalledConfiguration may require further functionality/persistence to adjust the order.


> There will be some API refactoring/cleanup
> anyway (as you have already attempted).

And... :-) When should that occur, and who is gonna do it?


> I wonder if this patch can simply
> define the Configuration.findServices method that might well be an
> abstract method implemented by the InstalledConfiguration class
> which will only be an implementation class used by StoredConfiguration.
>

IMHO that is breaking the abstraction since service provider modules are not linked at compile time.

What would be the implementation of Configuration.findServices for a type of Configuration?

--

I did not want to mess around with StoredConfiguration knowing that it will change. Currently it needs to use the same type for reading/writing (as per the abstraction StoredConfiguration extends from).

FWIW this was the "purest" abstraction i could think of without doing further refactoring [1], so i think we should reconsider abstracting run-time and install-time configuration, if it makes sense, as part of a larger refactoring effort, or as part of other patches if it makes sense within that context.

Maybe renaming to RunAndInstallConfiguration would help?

Paul.

[*] Javac does a similar trick with Configuration and a special implementation of Catalog, com.sun.tools.javac.jigsaw.JavacCatalog.

[1] I also wanted to avoid pushing this explicitly into the runtime, say in LoaderPool, as the implementation of the Iterable could change over time depending on how things are stored/ordered etc.)