RFR: JDK-8073152: Update Standard/ExtendedCharsets to work with module system
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. The proposed change here is to re-organize the way these two providers are built, and put in the mechanism that enable the build system to configure which group of charsets need to be moved from the "extended" charsets into the "standard" (java.base), based on the target platform, during the jdk build time. The updates are (1) to consolidate the existing configuration files "sbcs", "dbcs", "extsbcs" and "standard-charsets" (and the aliases table originally hardcoded in ExtendeardCharsets.java) into a single configuration file "charsets" to define all supported charsets, with their name, clz name, history name, package name and all aliases http://cr.openjdk.java.net/~sherman/8073152/webrev/make/data/charsetmapping/... (2) in addition to the the "standard charsets" provider, now the "extended charsets" provider class is also built during the build time based on the new configuration file "charsets" (3) to add a build-time mechanism to specify individual sun.nio.cs.ext charset to be built into "standard" charsets provider (sun.nio.cs package in java.base module), with os-specific configuration file, such as stdcs-linux, stdcs-windows and stdcs-solaris. (4) to move DoubleByte, HKSCS and DelegatableDecoder classes into sun.nio.cs, so they can be used/referenced by those dbcs classes that might be moved into sun.nio.cs (5) to change some sun.nio.cs.ext implementation java files to be a ".template", so the real .java can be generated during build-time with correct pkg name and aliases info. There are two leftover, gb18030 and euc_tw, these are two charsets with huge mapping table. Might need special update to better fit into sun.nio.cs later. thanks! -Sherman
(adding build-dev) Hello Sherman, In Gensrc-jdk.charsets.gmk: * The variable CHARSET_EXTENDED is defined but never used. The value is used in several places however. I think you should either use the variable or not define it at all. * $(CHARSET_EXTENDED_DATA) is put on a prerequisite line, but the variable is not defined. * The recipe for $(CHARSET_DONE_CS)-extcs uses $(CHARSET_STANDARD_OS), $(CHARSET_EXTENDED_JAVA_TEMPLATES) and the directory $(CHARSET_EXTENDED_JAVA_DIR), but none of these are listed as prerequisites. I should think touching any of these files (and which files in $(CHARSET_EXTENDED_JAVA_DIR)?) should trigger a rebuild of $(CHARSET_DONE_CS)-extcs so they should be added to the prerequisites. In Gensrc-jdk.CharsetMapping.gmk: * The same issue with prerequisites not properly defined for $(CHARSET_DONE_BASE)-stdcs. $(CHARSET_STANDARD_OS), $(CHARSET_STANDARD_JAVA_TEMPLATES) and an unknown number of files in $(CHARSET_EXTSRC_DIR) should be added for correct incremental build behavior. /Erik On 2015-02-14 21: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. The proposed change here is to re-organize the way these two providers are built, and put in the mechanism that enable the build system to configure which group of charsets need to be moved from the "extended" charsets into the "standard" (java.base), based on the target platform, during the jdk build time.
The updates are
(1) to consolidate the existing configuration files "sbcs", "dbcs", "extsbcs" and "standard-charsets" (and the aliases table originally hardcoded in ExtendeardCharsets.java) into a single configuration file "charsets" to define all supported charsets, with their name, clz name, history name, package name and all aliases http://cr.openjdk.java.net/~sherman/8073152/webrev/make/data/charsetmapping/...
(2) in addition to the the "standard charsets" provider, now the "extended charsets" provider class is also built during the build time based on the new configuration file "charsets"
(3) to add a build-time mechanism to specify individual sun.nio.cs.ext charset to be built into "standard" charsets provider (sun.nio.cs package in java.base module), with os-specific configuration file, such as stdcs-linux, stdcs-windows and stdcs-solaris.
(4) to move DoubleByte, HKSCS and DelegatableDecoder classes into sun.nio.cs, so they can be used/referenced by those dbcs classes that might be moved into sun.nio.cs
(5) to change some sun.nio.cs.ext implementation java files to be a ".template", so the real .java can be generated during build-time with correct pkg name and aliases info.
There are two leftover, gb18030 and euc_tw, these are two charsets with huge mapping table. Might need special update to better fit into sun.nio.cs later.
thanks! -Sherman
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
On 2/16/15 4:11 AM, Alan Bateman wrote:
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.
Yes, we have a separate P2 bug for that, will address that separately later.
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.
No, it's not used by any code. I just added it the last minute to be a reference for the future, as I'm deleting the original list sbcs, dbcs and extsbcs.
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? "This" Hasher.java has a different use interface and works on a pair of key/value lists directly (instead of parsing the key/value from a std in). Given the implementation is simple/small enough, I just copied/ pasted the parts I need from Mark's code, left the original one untouched (I kinda remember it is used by other as well)
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);
Typo fixed. The "aliasValues" and "aliasKeys" pair is a little tricky. Two are supposed to collect the alias->csname pair for all charsets, with a mapping relation but in two separate lists (to pass to the Hasher). While it's possible to collect them separately (or them into a "entry" as a pair) with the .collect(), but it appears to me more nature to do just in once pass.
4. Are there any tests that need to be updated?
Only NIOCharsetAvailabilityTest.java. But I believe that test should not stop working for a while as it tried to "parse" the rt.jar and charsets.jar to collect the charset class names, which obviously does not work in module. It need to be rewritten with the new iterating mechanism. (it is supposed to be the last file in webrev. But it appears it was not there in first webrev)
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.
Phil is moving away from using those sun.nio.cs.ext directly. My change in motif should have no conflict with his changes, in fact they probably no longer needed. Just be there in case I will push first :-) http://cr.openjdk.java.net/~sherman/8073152/webrev -Sherman
-Alan
On 16/02/2015 18:46, Xueming Shen wrote:
:
1. Is list_old needed, I can't tell if this is checked in for use by future archaeologists.
No, it's not used by any code. I just added it the last minute to be a reference for the future, as I'm deleting the original list sbcs, dbcs and extsbcs. Okay, I'm just thinking of someone finding this file in 6 months or 6 years and wondering whether it is used in the build or not.
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? "This" Hasher.java has a different use interface and works on a pair of key/value lists directly (instead of parsing the key/value from a std in). Given the implementation is simple/small enough, I just copied/ pasted the parts I need from Mark's code, left the original one untouched (I kinda remember it is used by other as well)
Okay, just trying to reduce duplication.
:
4. Are there any tests that need to be updated?
Only NIOCharsetAvailabilityTest.java. But I believe that test should not stop working for a while as it tried to "parse" the rt.jar and charsets.jar to collect the charset class names, which obviously does not work in module. It need to be rewritten with the new iterating mechanism. (it is supposed to be the last file in webrev. But it appears it was not there in first webrev) Thanks, I was wondering if any tests were directly referencing charsets that moved on some platforms.
:
Phil is moving away from using those sun.nio.cs.ext directly. My change in motif should have no conflict with his changes, in fact they probably no longer needed. Just be there in case I will push first :-)
http://cr.openjdk.java.net/~sherman/8073152/webrev I think this looks okay.
-Alan
Am 16.02.2015 um 19:46 schrieb Xueming Shen:
On 2/16/15 4:11 AM, Alan Bateman wrote:
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? "This" Hasher.java has a different use interface and works on a pair of key/value lists directly (instead of parsing the key/value from a std in). Given the implementation is simple/small enough, I just copied/ pasted the parts I need from Mark's code, left the original one untouched (I kinda remember it is used by other as well)
Maybe consider: https://bugs.openjdk.java.net/browse/JDK-6850361 https://bugs.openjdk.java.net/browse/JDK-6862158 Here have been the patches: https://bugs.openjdk.java.net/show_bug.cgi?id=100095 https://bugs.openjdk.java.net/show_bug.cgi?id=100098 Thanks, -Ulf
participants (4)
-
Alan Bateman
-
Erik Joelsson
-
Ulf Zibis
-
Xueming Shen