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
