Using non-parallel custom class loaders for Layer configurations
Is it not necessary that any class loader in use by a Layer must be
parallel-capable? Otherwise it seems like deadlocks could occur in
certain situations when there are references that are cyclic with
respect to class loaders mapped by the mapping function.
On that note, there is in fact no good way for user code to determine
whether or not a class loader is indeed parallel-capable, which is often
a critical question in module systems, especially when users can provide
specialized class loader implementations. Right now you can only do
something (awful) like this (e.g. in your base ClassLoader constructor):
if (getClassLoadingLock("$TEST$") == this) {
throw new Error("Cannot instantiate non-parallel subclass");
}
Would it be possible to include a method like this (pretty old patch I
had laying around):
diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
index 1bb1580..3def10e 100644
--- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -469,6 +477,15 @@ public abstract class ClassLoader {
return lock;
}
+/**
+ * Determine whether this class loader is parallel-capable.
+ *
+ * @return {@code true} if the class loader is parallel-capable,
{@code false} otherwise
+ */
+protected final boolean isParallelCapable() {
+return ParallelLoaders.isRegistered(getClass());
+}
+
// This method is invoked by the virtual machine to load a class.
private Class loadClassInternal(String name)
throws ClassNotFoundException
Alternatively, the method could be made static and protected, only
returning true if the calling class is a class loader that is parallel
capable, something like this (against a newer branch):
diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
index 0139123..06a5909 100644
--- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -1421,6 +1421,19 @@ public abstract class ClassLoader {
}
/**
+ * Determine whether the calling class is a parallel-capable class
loader.
+ *
+ * @return true if the caller is a parallel-capable class loader
and false otherwise
+ *
+ * @since 9
+ */
+@CallerSensitive
+protected static boolean isParallelCapable() {
+final Class callerClass = Reflection.getCallerClass();
+return ClassLoader.class.isAssignableFrom(callerClass) &&
ParallelLoaders.isRegistered(callerClass.asSubclass(ClassLoader.class));
+}
+
+/**
* Find a resource of the specified name from the search path used
to load
* classes. This method locates the resource through the system class
* loader (see {@link #getSystemClassLoader()}).
WDYT?
--
- DML
Re: Using non-parallel custom class loaders for Layer configurations
> On Sep 8, 2016, at 3:29 PM, David M. Lloyd wrote:
>
> Would it be possible to include a method like this (pretty old patch I had
> laying around):
>
> diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
> b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
> index 1bb1580..3def10e 100644
> --- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
> +++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
> @@ -469,6 +477,15 @@ public abstract class ClassLoader {
> return lock;
> }
>
> +/**
> + * Determine whether this class loader is parallel-capable.
> + *
> + * @return {@code true} if the class loader is parallel-capable, {@code
> false} otherwise
> + */
> +protected final boolean isParallelCapable() {
> +return ParallelLoaders.isRegistered(getClass());
> +}
> +
This seems a fine method to add. I think it can be a public method (I don’t
see any reason it has to be protected)
Mandy
Re: Using non-parallel custom class loaders for Layer configurations
On 09/08/2016 06:48 PM, Mandy Chung wrote:
On Sep 8, 2016, at 3:29 PM, David M. Lloyd wrote:
Would it be possible to include a method like this (pretty old patch I had
laying around):
diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
index 1bb1580..3def10e 100644
--- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -469,6 +477,15 @@ public abstract class ClassLoader {
return lock;
}
+/**
+ * Determine whether this class loader is parallel-capable.
+ *
+ * @return {@code true} if the class loader is parallel-capable, {@code
false} otherwise
+ */
+protected final boolean isParallelCapable() {
+return ParallelLoaders.isRegistered(getClass());
+}
+
This seems a fine method to add. I think it can be a public method (I don’t
see any reason it has to be protected)
Great! Since it's such a small method though, would it be possible to
just roll this change into some other changeset? I can't really
contribute the change in any form other than a patch right now.
Thanks.
--
- DML
Re: Using non-parallel custom class loaders for Layer configurations
On 08/09/2016 23:29, David M. Lloyd wrote: Is it not necessary that any class loader in use by a Layer must be parallel-capable? Otherwise it seems like deadlocks could occur in certain situations when there are references that are cyclic with respect to class loaders mapped by the mapping function. There aren't cycles in readability graph, at least not at construction time. They might arise later due to reflective use of addReads but that shouldn't change the class loader delegation graph. So I'm curious if you are actually running into an issue or not? I can easily come up with a mapping to loaders to force this but it would be unusual mapping. BTW: I'm not opposed to exposing isParallelCapable, just curious if this is something that you are actually running into or not. -Alan
Re: Using non-parallel custom class loaders for Layer configurations
On 09/09/2016 11:26 AM, Alan Bateman wrote: On 08/09/2016 23:29, David M. Lloyd wrote: Is it not necessary that any class loader in use by a Layer must be parallel-capable? Otherwise it seems like deadlocks could occur in certain situations when there are references that are cyclic with respect to class loaders mapped by the mapping function. There aren't cycles in readability graph, at least not at construction time. They might arise later due to reflective use of addReads but that shouldn't change the class loader delegation graph. So I'm curious if you are actually running into an issue or not? I can easily come up with a mapping to loaders to force this but it would be unusual mapping. BTW: I'm not opposed to exposing isParallelCapable, just curious if this is something that you are actually running into or not. No, I've only run into this with our module system which is separate from Jigsaw, but I was examining this code recently and I was surprised to see that Jigsaw did not take this into account. I don't think there needs to be a readability cycle between modules for this to be a potential problem; there only has to be a cycle between class loaders. So if you (for whatever reason) put half of your modules in one class loader and half in another, and those modules refer to one another, I think you might have a problem if those modules refer to one another and the class loaders are not parallel capable. But like I said that's just a hypothesis. -- - DML
Re: Using non-parallel custom class loaders for Layer configurations
> On Sep 9, 2016, at 8:44 AM, David M. Lloyd wrote:
>
> On 09/08/2016 06:48 PM, Mandy Chung wrote:
>>
>>> On Sep 8, 2016, at 3:29 PM, David M. Lloyd wrote:
>>>
>>> Would it be possible to include a method like this (pretty old patch I had
>>> laying around):
>>>
>>> diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
>>> b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
>>> index 1bb1580..3def10e 100644
>>> --- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
>>> +++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
>>> @@ -469,6 +477,15 @@ public abstract class ClassLoader {
>>>return lock;
>>>}
>>>
>>> +/**
>>> + * Determine whether this class loader is parallel-capable.
>>> + *
>>> + * @return {@code true} if the class loader is parallel-capable,
>>> {@code false} otherwise
>>> + */
>>> +protected final boolean isParallelCapable() {
>>> +return ParallelLoaders.isRegistered(getClass());
>>> +}
>>> +
>>
>> This seems a fine method to add. I think it can be a public method (I don’t
>> see any reason it has to be protected)
>
> Great! Since it's such a small method though, would it be possible to just
> roll this change into some other changeset? I can't really contribute the
> change in any form other than a patch right now.
I file a RFE and Brent Christian has agreed to work on it and will go through
JDK 9 FC approval request.
https://bugs.openjdk.java.net/browse/JDK-8165793
Mandy
Re: Using non-parallel custom class loaders for Layer configurations
On 09/09/2016 02:05 PM, Mandy Chung wrote:
On Sep 9, 2016, at 8:44 AM, David M. Lloyd wrote:
On 09/08/2016 06:48 PM, Mandy Chung wrote:
On Sep 8, 2016, at 3:29 PM, David M. Lloyd wrote:
Would it be possible to include a method like this (pretty old patch I had
laying around):
diff --git a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
index 1bb1580..3def10e 100644
--- a/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
+++ b/jdk/src/java.base/share/classes/java/lang/ClassLoader.java
@@ -469,6 +477,15 @@ public abstract class ClassLoader {
return lock;
}
+/**
+ * Determine whether this class loader is parallel-capable.
+ *
+ * @return {@code true} if the class loader is parallel-capable, {@code
false} otherwise
+ */
+protected final boolean isParallelCapable() {
+return ParallelLoaders.isRegistered(getClass());
+}
+
This seems a fine method to add. I think it can be a public method (I don’t
see any reason it has to be protected)
Great! Since it's such a small method though, would it be possible to just
roll this change into some other changeset? I can't really contribute the
change in any form other than a patch right now.
I file a RFE and Brent Christian has agreed to work on it and will go through
JDK 9 FC approval request.
https://bugs.openjdk.java.net/browse/JDK-8165793
Thanks!
--
- DML
Re: Using non-parallel custom class loaders for Layer configurations
On 9/9/16 12:05 PM, Mandy Chung wrote: I file a RFE and Brent Christian has agreed to work on it and will go through JDK 9 FC approval request. https://bugs.openjdk.java.net/browse/JDK-8165793 FYI, the review thread is here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044060.html -Brent
