Bryan's comments

[Stanley M. Ho]

Hi Bryan,

Bryan Atsatt wrote:
> Stanley M. Ho wrote:
>>
>> Based on the updated specification, the module system will provide a
>> default version for the module definition which has no version declared
>> in the metadata, so each module definition will always have a version.
>> Is there any strong reason why we should reject all the module
>> definitions which have no version declared in the metadata rather than
>> giving them a default version number?
>
> It is more philosophical than technical: if developers are forced to
> declare it, they must be conscious of the implications of that choice.

I think that the barrier for developers to create a simple module should
be as low as possible. Making the version mandatory will make this
barrier higher. Also, many existing systems consider version to be
optional in their modules, probably for the same reason. Do you think
you can live with this if we continue to make version optional?

>>>>> Refactoring in this way results in a perhaps unexpected runtime/memory
>>>>> overhead. A pure wrapper module that re-exports must have its own
>>>>> class
>>>>> loader, and, even though it won't define any classes, the VM *cache*
>>>>> will be updated as that loader is used.
>>
>> If there is no support for re-exporting subset of imported
>> classes/resources, this becomes a non-issue.
>
> Sorry, but this doesn't follow. The VM class cache is updated (at least)
> when:
>
> loader.defineClass() is called.
> VM calls loader.internalLoadClass()
> Class.forName() is called.
>
> The cache records that the initiating loader returned a specific class,
> regardless of what loader defined it. This enables
> loader.findLoadedClass() to work as expected.
>
> Given that:
>
> a. Module has-a single ClassLoader, and
> b. A specific Module re-exports classes from an imported Module.
>
> we have two scenarios to consider:
>
> 1. Module has both member and re-exported classes. The loader will both
> define member classes and the VM cache will be updated with mappings
> from Module loader to imported-module-Class instances.
>
> 2. Module has no member classes, only re-exported classes. The cache
> will be updated with mappings from Module loader to
> imported-module-Class instances.
>
> In the latter case, if there is only a single imported module, an
> optimization could be that this view module returns the imported
> module's loader rather than having it's own.
>
> But this is not possible when there are imports from more than one
> module: a unique loader instance must be created to satisfy the api
> contract module.getClassLoader(). Even though it will never define any
> classes, the VM will still update the cache.
>
> This latter case is what I am referring to as overhead. Of course, with
> appropriate updates to the VM/caching behavior, this overhead could be
> eliminated. But this would have to be addressed or the overhead *will*
> exist.

Thanks for the explanation; I now understand your concern. One of our
strengths in this JSR is the ability to require deep changes in the
platform if necessary. If this particular classloading issue has
significant overhead, it should be addressed. I have forwarded your
concern to the classloading team for them to evaluate. If you can
provide any performance data related to this overhead to our
classloading team, it will be great.

> Ah. You are thinking that two different JVM vendors might implement this
> differently. Fair enough. But this means that the actual location of the
> expansion directory must also be defined by the JVM.

This is not limited to two different JVM vendors - even the same JVM
vendor can choose to use the same or different expansion directory for
two instances of local repository in the same session.

> But LocalRepository instances will likely need to be created by IDEs,
> and other environments. And they will want to control the location of
> the expansion directory. So I am suggesting that LocalRepository needs
> an appropriate ctor to enable the caller to specify the location of the
> expansion directory.
>
> I think it makes much more sense to assume that LocalRepository (and
> URLRepostory) has a constant, well-defined model. JVM vendors are free
> to create variant classes, and to instantiate those by default.

It sounds like what you really want is a way to hint the local
repository where it should expand its content if necessary. This seems
reasonable to me, and I will incorporate it into the next revision of
the specification.

> Ok, but the spec should at least say that. And there are some
> assumptions we are making about this mechanism, particularly that it
> will be possible to specify the runtime type of the repository classes.
> Shouldn't we say something about that as well?

No. If this is something that implementors are free to implement, it
does not need to be in the spec. I think describing it in the JDK
documentation would be sufficient for this purpose.

>>>>> 7.3.2.2
> ...
> I have two concerns:
>
> 1. I have seen a number of systems that *assume* a specific protocol
> (e.g. "jar"). They do this because they want to parse the url, either to
> construct new urls or to glean information from them (such as the jar
> path). This practice clearly limits implementors. The API contract of
> getResource/s does not say anything about the protocol, which makes
> perfect sense. But I think it should be even stronger, and say that
> callers *must not* assume anything about the protocol.
>
> 2. The "jar" protocol as implemented by the JRE includes a jar cache.
> This cache is problematic for two reasons:
>
> a. It forces a second ZipFile to be opened (the one managed by the
> cache). The repository will already have one open, so using this
> protocol is very inefficient.
>
> b. The lifecycle of the jars in the cache cannot be controlled by the
> repository implementor. On Windows, files cannot be deleted if they are
> open, so undeploy/uninstall can fail.
>
> So either we should clean up the jar protocol to eliminate these issues,
> or Repository implementations should be encouraged *not* to use it.
> Which makes #1 even more important.

Okay, I think we are now on the same page that it's not feasible to
define a common protocol for this in JSR 277. I see your point of #1,
and I will mention this "no protocol assumption" issue in the next
revision of the specification. #2 is a JDK implementation issue, and I
will forward this issue to the appropriate team in Java SE for evaluation.

- Stanley