Hi Claes,
I hope I'm not to late to comment on this. This change is ok as it
stands, but I'm afraid it is not in the spirit of Valhalla. As I
understand, you rely on the fact that Integer instances in the low range
of values are cached and you then use identity comparison in the
following method:
86 public ClassLoader apply(String name) {
87 Integer loader = map.get(name);
88 if (loader == APP_LOADER_INDEX) {
89 return APP_CLASSLOADER;
90 } else if (loader == PLATFORM_LOADER_INDEX) {
91 return PLATFORM_CLASSLOADER;
92 } else { // BOOT_LOADER_INDEX
93 return null;
94 }
95 }
Wouldn't it be better to rewrite this to something like the following:
57 /**
58 * Map from module to: (null: boot loader, FALSE: platform
loader, TRUE: app loader)
60 */
61 private final Map<String, Boolean> map;
if (loader == null) {
return null; // BOOT loader
} else if (loader) {
return APP_CLASSLOADER;
} else {
return PLATFORM_CLASSLOADER;
}
Regards, Peter
On 2/10/20 12:43 PM, Claes Redestad wrote:
On 2020-02-10 12:34, Alan Bateman wrote:
On 10/02/2020 09:04, Claes Redestad wrote:
:
So how about:
http://cr.openjdk.java.net/~redestad/8237878/open.02/
Thanks for restoring the use of Function<String, ClassLoader>.
Changing Module::defineClass to invoke a method on ModuleLoaderMap is
okay but the method needs to something like "isBuiltinMapper" because
it tests if the function is a built-in mapper used for the boot layer
(or child layers created when we need to dynamically augment the set
of platform modules).
Minor nit but I think the comment on the Mapper constructor would say
that it creates a Mapper to map the modules in the given
Configuration to the built-in class loaders.
The rest looks good to me.
Thanks!
I'll run a few tests and push with this addendum, then:
diff -r 43b98c0e075d src/java.base/share/classes/java/lang/Module.java
--- a/src/java.base/share/classes/java/lang/Module.java Mon Feb 10
12:40:49 2020 +0100
+++ b/src/java.base/share/classes/java/lang/Module.java Mon Feb 10
12:42:43 2020 +0100
@@ -1094,7 +1094,7 @@
// map each module to a class loader
ClassLoader pcl = ClassLoaders.platformClassLoader();
- boolean isModuleLoaderMapper =
ModuleLoaderMap.isModuleLoaderMapper(clf);
+ boolean isModuleLoaderMapper =
ModuleLoaderMap.isBuiltinMapper(clf);
for (int index = 0; index < numModules; index++) {
String name = resolvedModules[index].name();
diff -r 43b98c0e075d
src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
---
a/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
Mon Feb 10 12:40:49 2020 +0100
+++
b/src/java.base/share/classes/jdk/internal/module/ModuleLoaderMap.java
Mon Feb 10 12:42:43 2020 +0100
@@ -61,7 +61,8 @@
private final Map<String, Integer> map;
/**
- * Maps module names to the corresponding built-in classloader.
+ * Creates a Mapper to map module names in the given
Configuration to
+ * built-in classloaders.
*
* As a proxy for the actual classloader, we store an easily
archiveable
* index value in the internal map. The index is stored as a
boxed value
@@ -132,7 +133,7 @@
* to the boot or platform classloader if the ClassLoader mapping
function
* originate from here.
*/
- public static boolean isModuleLoaderMapper(Function<String,
ClassLoader> clf) {
+ public static boolean isBuiltinMapper(Function<String,
ClassLoader> clf) {
return clf instanceof Mapper;
}
}
/Claes
