Hi, this patch adds a check to see if there are any split packages in the system modules at link time, and uses this information to enable us to safely skip a runtime check during bootstrap for the common case that there are none of the sort.
Webrev[1]: http://cr.openjdk.java.net/~redestad/8171400/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8171400 This removes a chunk of the module system bootstrap overhead, and also amends a small issue where PACKAGES_IN_BOOT_LAYER would be wrong in the presence of split packages. Thanks! /Claes [1] Since cr.openjdk.java.net is down I've also attached the raw patch.
# HG changeset patch # User redestad # Date 1482149824 -3600 # Mon Dec 19 13:17:04 2016 +0100 # Node ID 0fd05985c26f78eba4ec8614f966d220b1f79ff0 # Parent ab164f8b856977122f61e9d2f0ce8272307f01b4 8171400: Move checking of duplicate packages in the boot layer to link time Reviewed-by: alanb diff --git a/src/java.base/share/classes/java/lang/reflect/Layer.java b/src/java.base/share/classes/java/lang/reflect/Layer.java --- a/src/java.base/share/classes/java/lang/reflect/Layer.java +++ b/src/java.base/share/classes/java/lang/reflect/Layer.java @@ -602,12 +602,8 @@ checkGetClassLoaderPermission(); - // For now, no two modules in the boot Layer may contain the same - // package so we use a simple check for the boot Layer to keep - // the overhead at startup to a minimum - if (boot() == null) { - checkBootModulesForDuplicatePkgs(cf); - } else { + // The boot layer is checked during module system initialization + if (boot() != null) { checkForDuplicatePkgs(cf, clf); } @@ -657,27 +653,6 @@ } /** - * Checks a configuration for the boot Layer to ensure that no two modules - * have the same package. - * - * @throws LayerInstantiationException - */ - private static void checkBootModulesForDuplicatePkgs(Configuration cf) { - Map<String, String> packageToModule = new HashMap<>(); - for (ResolvedModule resolvedModule : cf.modules()) { - ModuleDescriptor descriptor = resolvedModule.reference().descriptor(); - String name = descriptor.name(); - for (String p : descriptor.packages()) { - String other = packageToModule.putIfAbsent(p, name); - if (other != null) { - throw fail("Package " + p + " in both module " - + name + " and module " + other); - } - } - } - } - - /** * Checks a configuration and the module-to-loader mapping to ensure that * no two modules mapped to the same class loader have the same package. * It also checks that no two automatic modules have the same package. diff --git a/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java b/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java --- a/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java +++ b/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java @@ -308,6 +308,23 @@ } } + // if needed check that there are no split packages in the set of + // resolved modules for the boot layer + if (SystemModules.hasSplitPackages() || needPostResolutionChecks) { + Map<String, String> packageToModule = new HashMap<>(); + for (ResolvedModule resolvedModule : cf.modules()) { + ModuleDescriptor descriptor = + resolvedModule.reference().descriptor(); + String name = descriptor.name(); + for (String p : descriptor.packages()) { + String other = packageToModule.putIfAbsent(p, name); + if (other != null) { + fail("Package " + p + " in both module " + + name + " and module " + other); + } + } + } + } long t4 = System.nanoTime(); diff --git a/src/java.base/share/classes/jdk/internal/module/SystemModules.java b/src/java.base/share/classes/jdk/internal/module/SystemModules.java --- a/src/java.base/share/classes/jdk/internal/module/SystemModules.java +++ b/src/java.base/share/classes/jdk/internal/module/SystemModules.java @@ -57,6 +57,14 @@ public static int PACKAGES_IN_BOOT_LAYER = 1024; /** + * If there are no split packages in the run-time image, we can skip some + * checks during bootstrap. + */ + public static boolean hasSplitPackages() { + return true; + } + + /** * Returns a non-empty array of ModuleDescriptors in the run-time image. * * When running an exploded image it returns an empty array. diff --git a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java --- a/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java +++ b/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java @@ -34,6 +34,7 @@ import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -342,7 +343,8 @@ * * static Map<String, ModuleDescriptor> map = new HashMap<>(); */ - private void clinit(int numModules, int numPackages) { + private void clinit(int numModules, int numPackages, + boolean hasSplitPackages) { cw.visit(Opcodes.V1_8, ACC_PUBLIC+ACC_FINAL+ACC_SUPER, CLASSNAME, null, "java/lang/Object", null); @@ -379,6 +381,17 @@ clinit.visitInsn(RETURN); clinit.visitMaxs(0, 0); clinit.visitEnd(); + + // public static boolean hasSplitPackages(); + MethodVisitor split = + cw.visitMethod(ACC_PUBLIC+ACC_STATIC, "hasSplitPackages", + "()Z", null, null); + split.visitCode(); + split.visitInsn(hasSplitPackages ? ICONST_1 : ICONST_0); + split.visitInsn(IRETURN); + split.visitMaxs(0, 0); + split.visitEnd(); + } /* @@ -416,12 +429,16 @@ */ public ClassWriter getClassWriter() { int numModules = moduleInfos.size(); - int numPackages = 0; + Set<String> allPackages = new HashSet<>(); + int packageCount = 0; for (ModuleInfo minfo : moduleInfos) { - numPackages += minfo.packages.size(); + allPackages.addAll(minfo.packages); + packageCount += minfo.packages.size(); } - clinit(numModules, numPackages); + int numPackages = allPackages.size(); + boolean hasSplitPackages = (numPackages < packageCount); + clinit(numModules, numPackages, hasSplitPackages); // generate SystemModules::descriptors genDescriptorsMethod();