A compromise approach on module isolation
David M. Lloyd
david.lloyd at redhat.com
Mon May 15 14:12:45 UTC 2017
I've added a couple of commits [1] [2] that explore an alternative
approach where the property value can be a comma-separated list of
module names to isolate, or the special token "ALL" that puts all
application modules in isolated class loaders.
This allows fine-grained control over whether a module is isolated,
however it will not always be obvious which modules *need* to be
isolated (as the user may have to isolate modules that reference modules
with conflicting packages, in some cases). So I would expect that users
who run into isolation problems will use "ALL" unless they have special
memory requirements (class loaders are not free, but they're not
ginormous either, so for most people and most projects this is probably OK).
I also considered using a "ALL-MODULE-PATH" token instead as that
matches an existing convention, but thought maybe that is a bit more
specific than strictly necessary so I went for the shortest option
first. There is a possible weakness in the jlink part that a space in
the list could cause argument parsing problems. I didn't want to spend
time fixing that unless the basic concept is validated.
I envision that this could be translated to an "--isolate-modules="
command line argument.
[1] https://github.com/dmlloyd/openjdk-modules/commit/941c86f760654eb
[2] https://github.com/dmlloyd/openjdk-modules/commit/fb645cd681551eb
On 05/12/2017 04:09 PM, David M. Lloyd wrote:
> On 05/12/2017 03:38 PM, Tony Printezis wrote:
>> David, Remi, and all,
>>
>> I have a few questions (and a concern),
>>
>> - This seems like a straightforward change and I’m sure it must have
>> been proposed in the past. And I guess it was rejected for specific
>> reasons. What are the gotchas of this approach? I.e. Will it break
>> some code? Will it require some code to be changed?
>
> Hi Tony,
>
> Theoretically, any code which assumes that the class loader is going to
> be the same across modules will be disappointed if this setting is
> enabled. On the other hand, there are no modules yet, so one would hope
> that this would not be a problem; and also, the feature is switchable.
>
>> And will it bake-in some behavior that we might not want in the long
>> term?
>
> As long as it is switchable (and possibly non-normative), it's
> definitely impossible. Otherwise I don't know of any specific case
> where this would create a long-term issue (unless, hypothetically, it is
> a goal of the project to, say, eliminate ClassLoader altogether someday,
> or if some equally drastic long-term plan is in the works that I don't
> know of).
>
>> - (With my JVM/GC implementer hat on) I really don’t think launching a
>> class loader per module is a great idea. There are overheads in the
>> JVM for each class loader you introduce (performance - the GC has to
>> do some extra work per class loader, footprint - there are data
>> structures maintained per class loader and space could be wasted due
>> to fragmentation in the meta space of each class loader). FWIW, at
>> Twitter, most of our services do not use custom class loaders and we
>> get a lot of benefit out of that. It’d be a step backwards for us if
>> somehow our JVMs got bloated with potentially 1,000s of class loaders.
>
> It would be a tradeoff, for sure. We don't see too much negative impact
> derived from having many class loaders, but I agree that it is possible
> that a large number of them might cause an issue. I'm not sure that
> adding N class loaders on to N modules (which also have internal data
> structures and classes associated with them) will create a large %
> increase though; it would have to be tested. And of course it's
> switchable in the event that isolated package spaces are not needed.
>
> If there is in fact an impacting cost difference, another variation
> might be to allow isolation to be configured on a per-module basis
> (maybe using a module attribute or annotation, or a more complex
> command-line argument arrangement), or to allow the establishment of
> isolated groups, or other similar modifications. I believe these
> options should all be doable in a short time frame as well.
>
>> - Using class loaders is one way to isolate modules. Has anyone
>> considered alternatives? For example, doing automatic shading of
>> non-exported packages in the JVM (lots of hand-waving here)?
>
>
> One can always shade at build time, though this may be non-ideal for
> other reasons. This type of shading could be done at run time as you
> suggest, maybe using bytecode transformation, but that might be
> confusing for stack traces and so forth, and could possibly introduce
> other problems where class and package names vary unexpectedly. Still
> it could be a viable solution in some cases.
>
> Using class loaders is the only solution I can think of that doesn't
> introduce substantial overhead or involve rewriting the JVM's class
> loader implementation to redefine how it indexes packages (and updating
> the JVM and platform specs accordingly). At some point, adding
> complexity to reduce overhead may become zero-sum. :-) But I am of
> course open to other ideas as well.
>
>> On May 12, 2017 at 2:56:40 PM, David M. Lloyd (david.lloyd at redhat.com
>> <mailto:david.lloyd at redhat.com>) wrote:
>>
>>> It seems to work in my tests. Can you explain what you mean?
>>>
>>> $ jlink --module-path $JAVA_HOME/jmods:out/production --add-modules
>>> org.module.two,org.module.one --output new-test
>>> -J-Djdk.module.detect-cycles=false -J-Djdk.module.isolated=true
>>> --launcher one=org.module.one/org.module.one.Main --launcher
>>> two=org.module.two/org.module.two.Main
>>> $ new-test/bin/two
>>> This is module two, my
>>> cl=jdk.internal.loader.ClassLoaders$AppModuleClassLoader at 2b05039f
>>>
>>> On 05/12/2017 01:54 PM, Remi Forax wrote:
>>> > It doesn't work because application modules becomes system modules,
>>> > so even if you propagate the flags, an application modules becomes
>>> a > system module hence is not deployed in a class loader anymore.
>>> > > Rémi
>>> > > > On May 12, 2017 6:45:18 PM GMT+02:00, "David M. Lloyd" >
>>> <david.lloyd at redhat.com <mailto:david.lloyd at redhat.com>> wrote:
>>> > > Here's a commit which does so (and also covers the
>>> circularity case), if
>>> > you want to play with it a bit:
>>> > >
>>> https://github.com/dmlloyd/openjdk-modules/commit/ffbca0555aeb934181ac92ec3a2bf48fae7efe04
>>>
>>> > > On 05/12/2017 08:45 AM, David M. Lloyd wrote:
>>> > > Hi Rémi, thanks for the feedback.
>>> > > At the moment you can fix this by editing the launcher
>>> scripts and
>>> > adding "-Djdk.module.isolated=true" to the end of the
>>> > "JLINK_VM_OPTIONS=" line (and similarly for Windows).
>>> > > I think the tool can do this automatically with only minimal
>>> > changes.
>>> > I'll have a look at it.
>>> > > On 05/12/2017 03:28 AM, Remi Forax wrote:
>>> > > Hi David,
>>> > with this patch, there is an issue if people (like me :) )
>>> > are using
>>> > jlink.
>>> > > With jlink, application modules becomes system
>>> modules so
>>> > depending if
>>> > the application is delivered as a bunch of jars or as a
>>> > custom jlink
>>> > image, so behavior will be different because the
>>> application
>>> > modules
>>> > deploy with jlink will not be sandboxed.
>>> > > Maybe it means that jlink has to be changed, i do not
>>> know,
>>> > but i
>>> > think i would prefer that the module itself should decide
>>> > using the
>>> > module-info if it should run its own classloader or
>>> not, at
>>> > least
>>> > jlink can refuse to work if a module ask for being
>>> deployed
>>> > in its own
>>> > classloader
>>> > > And i'm still not sure that this kind of code has to
>>> be in
>>> > the jdk, it
>>> > can be easily written outside without putting the
>>> burden of
>>> > having to
>>> > maintain it in the JDK.
>>> > > regards,
>>> > Rémi
>>> > > > ----- Mail original -----
>>> > > De: "David M. Lloyd" <david.lloyd at redhat.com
>>> <mailto:david.lloyd at redhat.com>>
>>> > À: jpms-spec-experts at openjdk.java.net
>>> <mailto:jpms-spec-experts at openjdk.java.net>
>>> > Envoyé: Vendredi 12 Mai 2017 01:17:45
>>> > Objet: A compromise approach on module isolation
>>> > > > In the wake of the PR vote, one of the most
>>> oft-cited
>>> > unmet expectations
>>> > was the lack of package namespace isolation among
>>> > modules. I'd like to
>>> > propose a patch [1] which would allow this type of
>>> > module isolation to
>>> > be used by the run time on an opt-in or opt-out basis.
>>> > > Justification:
>>> > > • Large applications are more likely to encounter
>>> > concealed and
>>> > non-concealed package conflicts.
>>> > • By making the change opt-in or opt-out, any
>>> > application which expects
>>> > all of its modules to be loaded by a single class
>>> loader
>>> > can still
>>> > function, while allowing more complex applications
>>> > (which already would
>>> > necessarily have required at least some separated
>>> class
>>> > loaders) to
>>> > continue to work.
>>> > • The patch seems to have a small complexity and
>>> > maintenance footprint.
>>> > > Drawbacks:
>>> > > None that I am aware of.
>>> > > Patch information:
>>> > > The approach of this patch simply looks for a system
>>> > property and,
>>> > depending on the value of this property, establishes
>>> > isolated module
>>> > loaders. I relinquish any copyright claim to the patch
>>> > in this mail.
>>> > Please do not get bogged down by any formatting
>>> problems
>>> > introduced by
>>> > the mailing list; the purpose of directly including
>>> the
>>> > patch is to give
>>> > a clear, unambiguous subject for discussion. I can
>>> > provide a proper
>>> > webrev (or whatever other form is requested) if
>>> needed.
>>> > > Other than the system property (which is just one
>>> > possible approach), no
>>> > API or specification changes should be needed. This
>>> > version uses the
>>> > property "jdk.module.isolated" defaulting to "false".
>>> > > [1]:
>>> > > diff --git
>>> >
>>> a/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> > <http://ClassLoaders.java>
>>> >
>>> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> > <http://ClassLoaders.java>
>>> > index d976aab..f06adfb 100644
>>> > ---
>>> >
>>> a/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> > <http://ClassLoaders.java>
>>> > +++
>>> >
>>> b/jdk/src/java.base/share/classes/jdk/internal/loader/ClassLoaders.java
>>> > <http://ClassLoaders.java>
>>> > @@ -112,6 +112,15 @@ public static ClassLoader
>>> > appClassLoader() {
>>> > }
>>> > > /**
>>> > + * Returns a new isolated application module class
>>> loader.
>>> > + *
>>> > + * @return the new application module class loader
>>> (not
>>> > {@code
>>> > null})
>>> > + */
>>> > + public static ClassLoader appModuleClassLoader() {
>>> > + return new AppModuleClassLoader(APP_LOADER);
>>> > + }
>>> > +
>>> > + /**
>>> > * The class loader that is used to find resources
>>> in modules
>>> > defined to
>>> > * the boot class loader. It is not used for class
>>> loading.
>>> > */
>>> > @@ -220,6 +229,21 @@ protected Package
>>> > defineOrCheckPackage(String pn,
>>> > Manifest man, URL url) {
>>> > }
>>> > > /**
>>> > + * An isolated application module class loader
>>> that is
>>> > a {@code
>>> > BuiltinClassLoader} with
>>> > + * the application class loader as parent.
>>> > + */
>>> > + private static class AppModuleClassLoader extends
>>> > BuiltinClassLoader {
>>> > + static {
>>> > + if (!ClassLoader.registerAsParallelCapable())
>>> > + throw new InternalError();
>>> > + }
>>> > +
>>> > + AppModuleClassLoader(AppClassLoader parent) {
>>> > + super("app", parent, null);
>>> > + }
>>> > + }
>>> > +
>>> > + /**
>>> > * Returns a {@code URLClassPath} of file URLs to
>>> each of the
>>> > elements in
>>> > * the given class path.
>>> > */
>>> > diff --git
>>> >
>>> a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
>>>
>>> > <http://ModuleLoaderMap.java>
>>> > >
>>> b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
>>>
>>> > <http://ModuleLoaderMap.java>
>>> > > index b0f465d..cc2bdb6 100644
>>> > ---
>>> >
>>> a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
>>>
>>> > <http://ModuleLoaderMap.java>
>>> > > +++
>>> >
>>> b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
>>>
>>> > <http://ModuleLoaderMap.java>
>>> > > @@ -85,6 +85,8 @@ public ClassLoader apply(String
>>> name) {
>>> > return platformModules;
>>> > }
>>> > > + private static final boolean
>>> ISOLATED_MODULE_PATH =
>>> >
>>> Boolean.parseBoolean(System.getProperty("jdk.module.isolated",
>>> > > "false"));
>>> > +
>>> > /**
>>> > * Returns the function to map modules in the given
>>> > configuration
>>> > to the
>>> > * built-in class loaders.
>>> > @@ -102,6 +104,8 @@ public ClassLoader apply(String
>>> name) {
>>> > if (!bootModules.contains(mn)) {
>>> > if (platformModules.contains(mn)) {
>>> > map.put(mn, platformClassLoader);
>>> > + } else if (ISOLATED_MODULE_PATH) {
>>> > + map.put(mn, ClassLoaders.appModuleClassLoader());
>>> > } else {
>>> > map.put(mn, appClassLoader);
>>> > }
>>> > > > -- > - DML
>>> > > > > > > -- > Sent from my Android device with K-9 Mail. Please
>>> excuse my brevity.
>>>
>>>
>>> --
>>> - DML
>
>
--
- DML
More information about the jpms-spec-experts
mailing list