> 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 ?
