> On 11 Mar 2016, at 10:39, Alan Bateman <alan.bate...@oracle.com> wrote: > > > I've refreshed the webrevs here: > http://cr.openjdk.java.net/~alanb/8142968/2 >
I browsed the code. Generally the feeling i get is it’s of good quality. Previously some of this area was quite legacy and it is refreshing to view more modern code. Comments below, only the first is something i think we should really change, the others are up to you. Once Jigsaw goes in i wonder if we should opportunistically revisit the use of URL and Enumeration with class loaders? Paul. sun/util/locale/provider/ResourceBundleProviderSupport.java — 72 // java.base may not be able to read the bundleClass's module. 73 PrivilegedAction<Void> pa1 = () -> { ctor.setAccessible(true); return null; }; 74 AccessController.doPrivileged(pa1); 75 try { 76 return ctor.newInstance((Object[]) null); 77 } catch (InvocationTargetException e) { 78 Unsafe.getUnsafe().throwException(e.getTargetException()); 79 } catch (InstantiationException | IllegalAccessException e) { 80 throw new InternalError(e); 81 } 82 } catch (NoSuchMethodException e) { 83 } 84 } 85 return null; 86 } Please avoid the use of Unsafe here, use a sneaky throw instead if necessary, or wrap. That Unsafe method needs to die! j.l.Class — 2476 @CallerSensitive 2477 public URL getResource(String name) { 2478 name = resolveName(name); 2479 2480 // if this Class and the caller are in the same named module 2481 // then attempt to get URL to the resource in the module 2482 Module module = getModule(); 2483 if (module.isNamed()) { 2484 Class<?> caller = Reflection.getCallerClass(); 2485 if (caller != null && caller.getModule() == module) { 2486 String mn = getModule().getName(); 2487 ClassLoader cl = getClassLoader0(); 2488 try { 2489 if (cl == null) { 2490 return BootLoader.findResource(mn, name); 2491 } else { 2492 return cl.findResource(mn, name); 2493 } 2494 } catch (IOException ioe) { 2495 return null; 2496 } 2497 } 2498 } The method getResourceAsStream has an optimisation for a BuiltinClassLoader: 2412 // special-case built-in class loaders to avoid the 2413 // need for a URL connection 2414 if (cl == null) { 2415 return BootLoader.findResourceAsStream(mn, name); 2416 } else if (cl instanceof BuiltinClassLoader) { 2417 return ((BuiltinClassLoader) cl).findResourceAsStream(mn, name); 2418 } else { 2419 URL url = cl.findResource(mn, name); 2420 return (url != null) ? url.openStream() : null; 2421 } Is that something you plan to include for getResource too at some point? j.l.ClassLoader — 1777 // define Package object if the named package is not yet defined 1778 NamedPackage p = packages.computeIfAbsent(name, 1779 k -> NamedPackage.toPackage(name, m)); 1780 if (p instanceof Package) 1781 return (Package)p; 1782 1783 // otherwise, replace the NamedPackage object with Package object 1784 return (Package)packages.compute(name, this::toPackage); You could merge the computeIfAbsent and compute into a single compute call if you so wish. return (Package)packages.compute((n, p) -> { // define Package object if the named package is not yet defined if (p == null) return NamedPackage.toPackage(name, m); // otherwise, replace the NamedPackage object with Package object return (p instanceof Package) ? p : toPackage(n, p); }); 2553 /** 2554 * Returns the ServiceCatalog for modules defined to this class loader, 2555 * creating it if it doesn't already exist. 2556 */ 2557 ServicesCatalog createOrGetServicesCatalog() { 2558 ServicesCatalog catalog = servicesCatalog; 2559 if (catalog == null) { 2560 catalog = new ServicesCatalog(); 2561 boolean set = trySetObjectField("servicesCatalog", catalog); 2562 if (!set) { 2563 // beaten by someone else 2564 catalog = servicesCatalog; 2565 } 2566 } 2567 return catalog; 2568 } 2569 2570 // the ServiceCatalog for modules associated with this class loader. 2571 private volatile ServicesCatalog servicesCatalog; 2572 2573 2574 /** 2575 * Attempts to atomically set a volatile field in this object. Returns 2576 * {@code true} if not beaten by another thread. Avoids the use of 2577 * AtomicReferenceFieldUpdater in this class. 2578 */ 2579 private boolean trySetObjectField(String name, Object obj) { 2580 Unsafe unsafe = Unsafe.getUnsafe(); 2581 Class<?> k = ClassLoader.class; 2582 long offset; 2583 try { 2584 Field f = k.getDeclaredField(name); 2585 offset = unsafe.objectFieldOffset(f); 2586 } catch (NoSuchFieldException e) { 2587 throw new InternalError(e); 2588 } 2589 return unsafe.compareAndSwapObject(this, offset, null, obj); 2590 } If you can syncrhonize on this, just use double checked locking? j.l.i.BoundMethodHandle — 791 // ## FIXME 792 // assert F_SPECIES_DATA.getDeclaredAnnotation(Stable.class) != null; That’s odd. Why did that need commenting out? MethodHandles — That’s quite some trick to create PUBLIC_LOOKUP :-) spinning up a new class (“Unamed”) in a new class loader. If that mechanism is gonna stay and If there are costs associated with that, we could easily skip the ASM part and use hard coded bytes. j.l.r.Proxy — 636 private static final WeakCache<Module, List<Class<?>>, Class<?>> proxyCache = 637 new WeakCache<>(new KeyFactory<Module>(), 638 new BiFunction<Module, List<Class<?>>, Class<?>>() { 639 @Override 640 public Class<?> apply(Module m, List<Class<?>> interfaces) { 641 Objects.requireNonNull(m); 642 return defineProxyClass(m, interfaces); 643 } 644 }); Wild idea: this feels like an equivalent of j.l.i.ClassValue for Module. 781 static Set<Class<?>> referencedTypes(ClassLoader loader, List<Class<?>> interfaces) { 782 Set<Class<?>> types = new HashSet<>(); 783 for (Class<?> intf : interfaces) { 784 for (Method m : intf.getMethods()) { 785 // return type, parameter types, and exception types 786 Stream.concat(Stream.concat(Stream.of(m.getReturnType()), 787 Arrays.stream(m.getParameterTypes())), 788 Arrays.stream(m.getExceptionTypes())) 789 .map(ProxyBuilder::getElementType) 790 .filter(t -> !t.isPrimitive()) 791 .forEach(types::add); 792 } 793 } 794 return types; 795 } Use Stream.flatMap e.g. interfaces.stream().flatMap(i -> Stream.of(i.getMethods()). flatMap(m -> f(m) /*extract all types on method*/). map(…).filter(…).collect(toSet()) Stream<Class<?>> f(Method m) { return Stream.of(new Class[] {m.getReturnType()}, m.getParameterTypes(), m.getExceptionTypes()). flatMap(a -> Stream.of(a)); } 939 private static final WeakHashMap<ClassLoader, Module> dynProxyModules = new WeakHashMap<>(); 940 private static final AtomicInteger counter = new AtomicInteger(); 941 942 /* 943 * Define a dynamic module for the generated proxy classes in a non-exported package 944 * named com.sun.proxy.$MODULE. 945 * 946 * Each class loader will have one dynamic module. 947 */ 948 static synchronized Module getDynamicModule(ClassLoader loader) { 949 Module m = dynProxyModules.get(loader); 950 if (m == null) { 951 String mn = "jdk.proxy" + counter.incrementAndGet(); 952 String pn = PROXY_PACKAGE_PREFIX + "." + mn; 953 m = Modules.defineModule(loader, mn, Collections.singleton(pn)); 954 Modules.addReads(m, Proxy.class.getModule()); 955 // java.base to create proxy instance 956 Modules.addExports(m, pn, Object.class.getModule()); 957 dynProxyModules.put(loader, m); 958 } 959 return m; 960 } Consider using computeIfAbsent. At the moment no need for AtomicInteger. 1055 @CallerSensitive 1056 public static Object newProxyInstance(ClassLoader loader, 1057 Class<?>[] interfaces, 1058 InvocationHandler h) { 1059 Objects.requireNonNull(h); 1060 1061 final List<Class<?>> intfs = Arrays.asList(interfaces.clone()); Use List.of, which will also clone? This presumes that there are no null values in the array. sun.invoke.util.VerifyAccess — 198 while (c.isArray()) { 199 c = c.getComponentType(); 200 } This pattern is occurring a number of times. Memo: add a method on Arrays, or somewhere…, sun.net.www.protocol.jrt.JavaRuntimeURLConnection — 56 private static final ImageReader reader = ImageReaderFactory.getImageReader(); reader -> READER sun.util.resources.LocaleData — 77 private static final Map<String, List<Locale>> candidatesMap = new ConcurrentHashMap<>(); candidatesMap -> CANDIDATES_MAP native/libjimage/imageDecompressor.cpp — 114 // Sparc to read unaligned content 115 // u8 l = (*(u8*) ptr); 116 // If ptr is not aligned, sparc will fail. 117 u8 ImageDecompressor::getU8(u1* ptr, Endian *endian) { 118 u8 ret; 119 if (endian->is_big_endian()) { 120 ret = (u8)ptr[0] << 56 | (u8)ptr[1] << 48 | (u8)ptr[2]<<40 | (u8)ptr[3]<<32 | 121 ptr[4]<<24 | ptr[5]<<16 | ptr[6]<<8 | ptr[7]; 122 } else { 123 ret = ptr[0] | ptr[1]<<8 | ptr[2]<<16 | ptr[3]<<24 | (u8)ptr[4]<<32 | 124 (u8)ptr[5]<<40 | (u8)ptr[6]<<48 | (u8)ptr[7]<<56; 125 } 126 return ret; 127 } 128 129 u4 ImageDecompressor::getU4(u1* ptr, Endian *endian) { 130 u4 ret; 131 if (endian->is_big_endian()) { 132 ret = ptr[0] << 24 | ptr[1]<<16 | (ptr[2]<<8) | ptr[3]; 133 } else { 134 ret = ptr[0] | ptr[1]<<8 | (ptr[2]<<16) | ptr[3]<<24; 135 } 136 return ret; 137 } I dunno if hotspot/src/share/vm/utilities/bytes.hpp can be leveraged here. java.lang.module.ModuleInfo — 460 private static boolean isAttributeDisallowed(String name) { 461 Set<String> notAllowed = predefinedNotAllowed; 462 if (notAllowed == null) { 463 notAllowed = Stream.of( 464 "ConstantValue", 465 "Code", 466 "StackMapTable", 467 "Exceptions", 468 "EnclosingMethod", 469 "Signature", 470 "LineNumberTable", 471 "LocalVariableTable", 472 "LocalVariableTypeTable", 473 "RuntimeVisibleAnnotations", 474 "RuntimeInvisibleAnnotations", 475 "RuntimeVisibleParameterAnnotations", 476 "RuntimeInvisibleParameterAnnotations", 477 "RuntimeVisibleTypeAnnotations", 478 "RuntimeInvisibleTypeAnnotations", 479 "AnnotationDefault", 480 "BootstrapMethods", 481 "MethodParameters") 482 .collect(Collectors.toSet()); 483 predefinedNotAllowed = notAllowed; Consider using Set.of 484 } 485 486 if (notAllowed.contains(name)) { 487 return true; 488 } else { 489 return false; 490 } 491 } j.l.ModuleReader — 145 default Optional<ByteBuffer> read(String name) throws IOException { 146 final int BUFFER_SIZE = 8192; 147 final int MAX_BUFFER_SIZE = Integer.MAX_VALUE - 8; 148 149 InputStream in = open(name).orElse(null); 150 if (in == null) { 151 // not found 152 return Optional.empty(); 153 } 154 155 try (in) { 156 int capacity = in.available(); 157 if (capacity == 0) 158 capacity = BUFFER_SIZE; 159 160 byte[] buf = new byte[capacity]; 161 int nread = 0; 162 int n; 163 for (;;) { Use InputStream.readAllBytes? jdk.internal.module.Builder — 56 private static final Set<Requires.Modifier> MANDATED = 57 Collections.singleton(Requires.Modifier.MANDATED); 58 private static final Set<Requires.Modifier> PUBLIC = 59 Collections.singleton(Requires.Modifier.PUBLIC); Set.of ?