A compromise approach on module isolation
David M. Lloyd
david.lloyd at redhat.com
Mon May 15 14:14:33 UTC 2017
Corrected link for [2]:
https://github.com/dmlloyd/openjdk-modules/commit/117a1cd6a8c1c44715
On 05/15/2017 09:12 AM, David M. Lloyd wrote:
> 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