On 14/02/2015 20:30, Xueming Shen wrote:
Hi,
Please help review the changes for JDK-8073152
Issue: https://bugs.openjdk.java.net/browse/JDK-8073152 Webrev: http://cr.openjdk.java.net/~sherman/8073152/webrev/
This change is to re-organize how the "standard" and "extended" charsets repository are built for the jdk/jre images. Before module system, the "standard" charset provider is built into rt.jar and the "extended" charsets provider is in charsets.jar, which is located in the "lib/ext" directory. both are available during the system boot time. In jigsaw, the extended charsets provider is in jdk.charset (vs the "standard" is in java.base) module and might not be "visible" during the early phase of system classes loading, which might cause problem as some of the charsets now in "extended" charsets may needed during the boot stage. Right, the overall goal is to ensure that we only execute in code java.base during startup (essentially until VM.isBooted returns true) so this means fixing a number of issues.
Another thing to say is that the compatibility impact will be that attempting to start with file.encoding set to a charset that remains in jdk.charset will not work but then again, startup up with this property set has never been supported anyway. The other impact for embedded environments is increased static footprint of java.base but I think that can be dealt with via filtering when creating the runtime image for the target platform. So overall then I think these changes look good. At some point I think that the private Charset.lookupExtendedCharset should fail if VM.isBooted() is false. Also I think ExtendedProviderHolder should go away and the extended charsets loaded via ServiceLoader. Another thing that comes to mind is the Charset.isSupported and forName methods, we might need to think about adding a note to their javadoc to say that some charsets might not be available during startup. Chris is penning similar text for URL protocol handlers. A few small comments about the changes in the webrev: 1. Is list_old needed, I can't tell if this is checked in for use by future archaeologists. 2. Hasher.java is showing up in the webrev as a new file, was this build.tools.hasher.Hasher and so we know have two copies? 3. In Typo in SPI.java line 136 ("Unknow type"). As this is a built-time tool then it's not a big deal but it is possible to use the streams API in more succulent ways, for example List<String> aliasValues = cs.aliases().stream().map(a -> a.toLowerCase(Locale.ENGLISH)).collect(Collectors::toList); 4. Are there any tests that need to be updated? Finally, I see the sun.awt.motif.** classes are changed to importing using wildcard, a subtle trick as the package name gets decided at build time. Hopefully someone in the client area can sort this dependency issue soon. -Alan