RFR: 8237878: Improve ModuleLoaderMap datastructures

[Peter Levart]

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