A compromise approach on module isolation
David M. Lloyd
david.lloyd at redhat.com
Fri May 12 21:09:54 UTC 2017
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