Module views with exports support

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Module views with exports support

Mandy Chung
A new webrev for the module views [1] with the exports support:
    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/

High-level summary:
   - Introduced a new ModuleView class and Service.Dependence
     Service.ProviderInfo classes. Each ModuleInfo has one or
     more views.
   - Renamed ModuleIdQuery to ModuleViewQuery
   - Renamed methods in the Catalog class to return ModuleId for
     module views.  E.g.
       gatherLocalModuleIds ->  gatherLocalModuleViewIds
       listModuleIds        ->  listModuleViewIds
       findModuleIds        ->  findModuleViewIds
       findLatestModuleId   ->  findLatestModuleViewId
   - Linker is updated to link with types that are exported by
     its dependence and only exported set of remote packages are
     stored in the configuration.

I also considered renaming ModuleId to ModuleViewId (not done in
the above webrev) but another thought would be to keep ModuleId
to represents a general identifier. I'd like to get your opinion
for this refactoring/renaming before moving forward.

With exports, a module can access types exported from a module
view that it requires.  A configuration has a set of contexts
and each context has a list of local classes and also a map
from a package name to a remote context.  Before exports, all
public types are exported. For a simple application that requires
the "jdk" module, the configuration size is reduced by 73% (6.3)

  8.6M   mlib-tip/com.greetings/0.1/config
  2.3M   mlib-views/com.greetings/0.1/config

If it requires jdk.base, the configuration size is much smaller
as jdk.base has fewer public classes than jdk.  With exports,
the size is reduced by 8.7% (72K).

  824K   mlib-base-tip/com.greetings/0.1/config
  752K   mlib-base-views/com.greetings/0.1/config

This is the module-info.java requiring the jdk module
(the current default platform module).
----------------------
module com.greetings @ 0.1 {
     requires org.astro;
     class com.greetings.Hello;
}

module org.astro @ 2.0 {
     exports org.astro;
}
----------------------------

For the JDK modules with exports [2], the system module library size
is reduced by 8M (7.6%):

  105M   jigsaw-repo/jdk/build/linux-i586/lib/modules
   97M   jigsaw-views/jdk/build/linux-i586/lib/modules

It is still big in the current prototype and we expect that the fast
configuration work will further reduce the footprint (this is work in
progress).  The number here is just to compare the size before and
after with exports support.

Thanks
Mandy

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-December/001813.html
[2] http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/module-info/jdk-modules-with-views.txt
[3] http://cr.openjdk.java.net/~mchung/jigsaw/views-api/

Reply | Threaded
Open this post in threaded view
|

Re: Module views with exports support

Alan Bateman
On 30/12/2011 17:42, Mandy Chung wrote:

> A new webrev for the module views [1] with the exports support:
>    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/
>
> High-level summary:
>   - Introduced a new ModuleView class and Service.Dependence
>     Service.ProviderInfo classes. Each ModuleInfo has one or
>     more views.
>   - Renamed ModuleIdQuery to ModuleViewQuery
>   - Renamed methods in the Catalog class to return ModuleId for
>     module views.  E.g.
>       gatherLocalModuleIds ->  gatherLocalModuleViewIds
>       listModuleIds        ->  listModuleViewIds
>       findModuleIds        ->  findModuleViewIds
>       findLatestModuleId   ->  findLatestModuleViewId
>   - Linker is updated to link with types that are exported by
>     its dependence and only exported set of remote packages are
>     stored in the configuration.
>
> I also considered renaming ModuleId to ModuleViewId (not done in
> the above webrev) but another thought would be to keep ModuleId
> to represents a general identifier. I'd like to get your opinion
> for this refactoring/renaming before moving forward.
>
> With exports, a module can access types exported from a module
> view that it requires.  A configuration has a set of contexts
> and each context has a list of local classes and also a map
> from a package name to a remote context.  Before exports, all
> public types are exported. For a simple application that requires
> the "jdk" module, the configuration size is reduced by 73% (6.3)
>
>  8.6M   mlib-tip/com.greetings/0.1/config
>  2.3M   mlib-views/com.greetings/0.1/config
>
> If it requires jdk.base, the configuration size is much smaller
> as jdk.base has fewer public classes than jdk.  With exports,
> the size is reduced by 8.7% (72K).
>
>  824K   mlib-base-tip/com.greetings/0.1/config
>  752K   mlib-base-views/com.greetings/0.1/config
>
> This is the module-info.java requiring the jdk module
> (the current default platform module).
> ----------------------
> module com.greetings @ 0.1 {
>     requires org.astro;
>     class com.greetings.Hello;
> }
>
> module org.astro @ 2.0 {
>     exports org.astro;
> }
> ----------------------------
>
> For the JDK modules with exports [2], the system module library size
> is reduced by 8M (7.6%):
>
>  105M   jigsaw-repo/jdk/build/linux-i586/lib/modules
>   97M   jigsaw-views/jdk/build/linux-i586/lib/modules
>
> It is still big in the current prototype and we expect that the fast
> configuration work will further reduce the footprint (this is work in
> progress).  The number here is just to compare the size before and
> after with exports support.
>
> Thanks
> Mandy
>
> [1]
> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-December/001813.html
> [2]
> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/module-info/jdk-modules-with-views.txt
> [3] http://cr.openjdk.java.net/~mchung/jigsaw/views-api/
>
I've taken a first pass over this, lots of good work.

I like views and works really well for the JDK. I'm in two minds on some
of the renaming as it forces anyone using the API to have to deal with
the concept more than I had expected. For example when one invokes a
list or find modules in a Catalog then I think it's debatable as to
whether it should return the ModuleIds of the modules or ModuleIds
synthesized from the view name and the module's version.

I'm also in two minds on the changes to the module library where there
is a directory with a copy of the module info. I can see how this is
used but it kinda feels that this should be an index (key'ed on view)
instead.

In ModuleInfo then defaultView and views() make sense (the views method
just needs to be clear that the returned set includes the default view).
I'm less sure about the view(ModuleId) and view(String) methods as they
aren't strictly required.

In Service.Dependence I see the constructor takes a boolean to indicate
if the dependency is optional. It may be more consistent (with
Dependence) to have it take a set of modifiers - if you agree then I can
take into the services patch as it's not re-based to your changes.

I don't see anything obviously wrong with the exports work although I
found ContextView in the linker hard to deal, maybe it just needs
another name. What you would think about jmod ls -v printing the exports
too?

On the configuration size then the exports will reduce the size as you
say but the size is a still a problem. I did a quick analysis of a hello
world app that has all JDK modules installed. It looks like about 68% of
the size is the local class map, and 30% for the remote package map. We
can easily index these and it should reduce the size considerably.

Minor nit is that I didn't understand the changes to Hi :-)

-Alan.






Reply | Threaded
Open this post in threaded view
|

Re: Module views with exports support

mark.reinhold
In reply to this post by Mandy Chung
2011/12/30 9:42 -0800, [hidden email]:
> A new webrev for the module views [1] with the exports support:
>    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/

There are some big changes here, thanks for working through all the
details.

Suggestions on the API:

  - Rename ModuleViewQuery back to ModuleIdQuery, for now.

  - Rename Dependence to ViewDependence, rename Service.Dependence to
    ServiceDependence, and make both of them extend a new class
    Dependence which declares only the modifiers() method.

  - Replace Service.ProviderInfo with a map in the ModuleView interface:
    Map<String,Set<String>> ModuleView.services().

  - Delete the now-empty Service class.

  - Rename ModuleInfo.requires() to .requiresModules().

  - In ModuleInfo, is the view(ModuleId) method really necessary?  If so
    then replace it and theand views() methods with a single method:
    Map<ModuleId,View> views().  If not, remove it.

  - In ModuleInfo, is the views(String) method really necessary?  If not,
    remove it.

The implementation is fine for now, though as previously discussed we
need eventually to integrate views more deeply into the code so that it's
not necessary to have multiple copies of the same module-info file in a
library when a module has multiple views.

Finally, I see that the unit tests use wildcard exports ("exports y.*")
rather than plain package exports ("exports y").  Is this temporary,
waiting on related changes in javac?

- Mark
Reply | Threaded
Open this post in threaded view
|

Re: Module views with exports support

Mandy Chung
Thanks for the review and suggestion.  I'll make the change and send out
a new webrev.

W.r.t. the the wildcard exports, it's temporary and waiting for javac
related change.

Mandy

On 1/24/2012 9:56 AM, [hidden email] wrote:

> 2011/12/30 9:42 -0800, [hidden email]:
>> A new webrev for the module views [1] with the exports support:
>>     http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.01/
> There are some big changes here, thanks for working through all the
> details.
>
> Suggestions on the API:
>
>    - Rename ModuleViewQuery back to ModuleIdQuery, for now.
>
>    - Rename Dependence to ViewDependence, rename Service.Dependence to
>      ServiceDependence, and make both of them extend a new class
>      Dependence which declares only the modifiers() method.
>
>    - Replace Service.ProviderInfo with a map in the ModuleView interface:
>      Map<String,Set<String>>  ModuleView.services().
>
>    - Delete the now-empty Service class.
>
>    - Rename ModuleInfo.requires() to .requiresModules().
>
>    - In ModuleInfo, is the view(ModuleId) method really necessary?  If so
>      then replace it and theand views() methods with a single method:
>      Map<ModuleId,View>  views().  If not, remove it.
>
>    - In ModuleInfo, is the views(String) method really necessary?  If not,
>      remove it.
>
> The implementation is fine for now, though as previously discussed we
> need eventually to integrate views more deeply into the code so that it's
> not necessary to have multiple copies of the same module-info file in a
> library when a module has multiple views.
>
> Finally, I see that the unit tests use wildcard exports ("exports y.*")
> rather than plain package exports ("exports y").  Is this temporary,
> waiting on related changes in javac?
>
> - Mark
Reply | Threaded
Open this post in threaded view
|

Re: Module views with exports support

Mandy Chung
In reply to this post by mark.reinhold
Updated webrev at:
   http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.02/

This incorporates Mark's and Alan's review comments.

On 1/24/2012 9:56 AM, [hidden email] wrote:
> Suggestions on the API:
>
>    - Rename ModuleViewQuery back to ModuleIdQuery, for now.
>
I also reverted the Category renamed methods and keep them as they are.
       gatherLocalModuleIds
       listModuleIds
       findModuleIds
       findLatestModuleId


>    - Rename Dependence to ViewDependence, rename Service.Dependence to
>      ServiceDependence, and make both of them extend a new class
>      Dependence which declares only the modifiers() method.
>    - Replace Service.ProviderInfo with a map in the ModuleView interface:
>      Map<String,Set<String>>  ModuleView.services().
>
>    - Delete the now-empty Service class.
>
>    - Rename ModuleInfo.requires() to .requiresModules().
>

Done

>    - In ModuleInfo, is the view(ModuleId) method really necessary?  If so
>      then replace it and theand views() methods with a single method:
>      Map<ModuleId,View>  views().  If not, remove it.
>

The view(ModuleId) is not strictly needed. It's used in the Resolver
when it first reads the ModuleInfo of the candidate and finds the
ModuleView of the given mid (L259-265).  It's also used in the new
Category.readModuleView convenient method.  I removed the view(ModuleId)
method for now and replaced the call site with the code to lookup the view.

>    - In ModuleInfo, is the views(String) method really necessary?  If not,
>      remove it.
>

removed.

On 1/17/2012 7:26 AM, Alan Bateman wrote:
> I like views and works really well for the JDK. I'm in two minds on
> some of the renaming as it forces anyone using the API to have to deal
> with the concept more than I had expected. For example when one
> invokes a list or find modules in a Catalog then I think it's
> debatable as to whether it should return the ModuleIds of the modules
> or ModuleIds synthesized from the view name and the module's version.

I keep them as they are for now.

> I'm also in two minds on the changes to the module library where there
> is a directory with a copy of the module info. I can see how this is
> used but it kinda feels that this should be an index (key'ed on view)
> instead.

Yep.  It's just an easy way to get this prototype implemented while
giving us more time to implement a long-term solution.  Will do that
next.

> In ModuleInfo then defaultView and views() make sense (the views
> method just needs to be clear that the returned set includes the
> default view). I'm less sure about the view(ModuleId) and view(String)
> methods as they aren't strictly required.

Removed.
> In Service.Dependence I see the constructor takes a boolean to
> indicate if the dependency is optional. It may be more consistent
> (with Dependence) to have it take a set of modifiers - if you agree
> then I can take into the services patch as it's not re-based to your
> changes.

I have revised it per Mark's suggestion.

> I don't see anything obviously wrong with the exports work although I
> found ContextView in the linker hard to deal, maybe it just needs
> another name.

I thought about naming it just 'View' :)

> What you would think about jmod ls -v printing the exports too?

I missed that in the webrev.  It's now included in the new version.


> Minor nit is that I didn't understand the changes to Hi :-)

That's left from my hack to use NetBeans to test jigsaw. I reverted
the change.

Thanks
Mandy



Reply | Threaded
Open this post in threaded view
|

Re: Module views with exports support

mark.reinhold
2012/1/24 20:43 -0800, [hidden email]:
> Updated webrev at:
>
>   http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/module-views.02/

Looks good to me.

- Mark