Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
Hi, On 2020-02-13 09:12, Peter Levart wrote: This change is ok as it stands, but I'm afraid it is not in the spirit of Valhalla. I wouldn't really know what the spirit of Valhalla will end up being, yet. :-) Unboxing the values have a measurable cost in the interpreter, and can trigger some very early JIT compilations I'd like to avoid. /Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
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 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.
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 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(FunctionClassLoader> clf) {
+ public static boolean isBuiltinMapper(FunctionClassLoader> clf) {
return clf instanceof Mapper;
}
}
/Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
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. 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 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(FunctionClassLoader> clf) {
+public static boolean isBuiltinMapper(Function
clf) {
return clf instanceof Mapper;
}
}
/Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
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. 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. -Alan
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
On 2020-02-09 17:49, Alan Bateman wrote: On 06/02/2020 13:48, Claes Redestad wrote: : New webrev: http://cr.openjdk.java.net/~redestad/8237878/open.01/ The archiving looks good but I'd prefer if this patch didn't change the usages of Function to ModuleLoaderMap.Mapper - that's an implementation class that should not be used outside of the ModuleLoaderMap (it should be private but there is special check in the Module code that prevents this). Ok, makes sense to make this private - I guess we could refactor the instanceof in Module to call a function on ModuleLoaderMap to encapsulate this better. Hoisting the check from the loop is a small optimization for the bootstrap case, regardless. The ModuleLoaderMap constructor needs a comment so that future maintainers know that it maps the modules in the given configuration to the built-in class loaders. Also would be good if the declaration of map were changed back to Map and a // comment to make it clear that it maps the module name to an index. So how about: http://cr.openjdk.java.net/~redestad/8237878/open.02/ /Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
On 06/02/2020 13:48, Claes Redestad wrote: : New webrev: http://cr.openjdk.java.net/~redestad/8237878/open.01/ The archiving looks good but I'd prefer if this patch didn't change the usages of Function to ModuleLoaderMap.Mapper - that's an implementation class that should not be used outside of the ModuleLoaderMap (it should be private but there is special check in the Module code that prevents this). The ModuleLoaderMap constructor needs a comment so that future maintainers know that it maps the modules in the given configuration to the built-in class loaders. Also would be good if the declaration of map were changed back to Map and a // comment to make it clear that it maps the module name to an index. -Alan
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
- Mail original - > De: "Claes Redestad" > À: "Remi Forax" > Cc: "core-libs-dev" > Envoyé: Jeudi 6 Février 2020 14:48:38 > Objet: Re: RFR: 8237878: Improve ModuleLoaderMap datastructures > Hi Rémi, > > > On 2020-02-06 14:08, Remi Forax wrote: >> Hi Claes, >> In ArchivedModuleGraph, there is no point to take the mainModule as parameter >> given that it should always be null. > > right, I've been thinking this should be simplified for now. Once we > implement support for archiving non-default scenarios the internal > datastructure will need a refactoring anyhow, so let's cut things down > to size. > >> >> In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX >> should be moved inside the nested class Mapper given that there are only >> needed >> by the Mapper >> and mappingFunction should be moved inside the class Mapper too. > > Done. > >> >> in mappingFunction, i think that if you are using 'var', you should using >> everywhere in the method, >> to avoid to have a weird mix between local variables declared with and >> without >> var. >> so either map is not declared with var or resolvedModule and nm are declared >> with var. > > I disagree var usage should be an all or nothing ordeal, rather it > should be used when the type information is readily available, e.g., > by being stated in detail on the RHS. > > The exact type in the other cases would be somewhat muddied: maybe > name() is "obviously" a String, but what type the elements in > cf.modules() are is not at all obvious, and making code less obvious > should be avoided. If your variable have a name like resolvedModule then the type ResolvedModule doesn't give you more info. Obviously, it doesn't work if the variable name is an abbreviation, but instead of not using var, i think it's better to change the variable name. > > New webrev: > > http://cr.openjdk.java.net/~redestad/8237878/open.01/ > > Re-running some tests.. > > /Claes Rémi > >> >> I believe that at some point in the future, let say just before the release >> of >> 17, we should do a global pass and rewrite each class to use 'var', the same >> way we have done with generics. >> It will be nasty for backporting bugs but i think it's better than having a >> mix >> between codes that use var and codes that doesn't use it. >> >> Rémi >> >> - Mail original - >>> De: "Claes Redestad" >>> À: "core-libs-dev" >>> Envoyé: Jeudi 6 Février 2020 13:37:59 >>> Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures >> >>> Hi, >>> >>> refactor ModuleLoaderMap to allow the datastructure to be >>> archived, then archive it. >>> >>> Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/ >>> Bug:https://bugs.openjdk.java.net/browse/JDK-8237878 >>> >>> Testing: tier1-3 >>> > >> /Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
Hi Rémi, On 2020-02-06 14:08, Remi Forax wrote: Hi Claes, In ArchivedModuleGraph, there is no point to take the mainModule as parameter given that it should always be null. right, I've been thinking this should be simplified for now. Once we implement support for archiving non-default scenarios the internal datastructure will need a refactoring anyhow, so let's cut things down to size. In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX should be moved inside the nested class Mapper given that there are only needed by the Mapper and mappingFunction should be moved inside the class Mapper too. Done. in mappingFunction, i think that if you are using 'var', you should using everywhere in the method, to avoid to have a weird mix between local variables declared with and without var. so either map is not declared with var or resolvedModule and nm are declared with var. I disagree var usage should be an all or nothing ordeal, rather it should be used when the type information is readily available, e.g., by being stated in detail on the RHS. The exact type in the other cases would be somewhat muddied: maybe name() is "obviously" a String, but what type the elements in cf.modules() are is not at all obvious, and making code less obvious should be avoided. New webrev: http://cr.openjdk.java.net/~redestad/8237878/open.01/ Re-running some tests.. /Claes I believe that at some point in the future, let say just before the release of 17, we should do a global pass and rewrite each class to use 'var', the same way we have done with generics. It will be nasty for backporting bugs but i think it's better than having a mix between codes that use var and codes that doesn't use it. Rémi - Mail original - De: "Claes Redestad" À: "core-libs-dev" Envoyé: Jeudi 6 Février 2020 13:37:59 Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures Hi, refactor ModuleLoaderMap to allow the datastructure to be archived, then archive it. Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/ Bug:https://bugs.openjdk.java.net/browse/JDK-8237878 Testing: tier1-3 /Claes
Re: RFR: 8237878: Improve ModuleLoaderMap datastructures
Hi Claes, In ArchivedModuleGraph, there is no point to take the mainModule as parameter given that it should always be null. In ModuleLoaderMap, the constants PLATFORM_LOADER_INDEX and APP_LOADER_INDEX should be moved inside the nested class Mapper given that there are only needed by the Mapper and mappingFunction should be moved inside the class Mapper too. in mappingFunction, i think that if you are using 'var', you should using everywhere in the method, to avoid to have a weird mix between local variables declared with and without var. so either map is not declared with var or resolvedModule and nm are declared with var. I believe that at some point in the future, let say just before the release of 17, we should do a global pass and rewrite each class to use 'var', the same way we have done with generics. It will be nasty for backporting bugs but i think it's better than having a mix between codes that use var and codes that doesn't use it. Rémi - Mail original - > De: "Claes Redestad" > À: "core-libs-dev" > Envoyé: Jeudi 6 Février 2020 13:37:59 > Objet: RFR: 8237878: Improve ModuleLoaderMap datastructures > Hi, > > refactor ModuleLoaderMap to allow the datastructure to be > archived, then archive it. > > Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/ > Bug:https://bugs.openjdk.java.net/browse/JDK-8237878 > > Testing: tier1-3 > > /Claes
RFR: 8237878: Improve ModuleLoaderMap datastructures
Hi, refactor ModuleLoaderMap to allow the datastructure to be archived, then archive it. Webrev: http://cr.openjdk.java.net/~redestad/8237878/open.00/ Bug:https://bugs.openjdk.java.net/browse/JDK-8237878 Testing: tier1-3 /Claes
