Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
On 01/12/2019 08:44, Ivan Gerasimov wrote: : Personally, I think that using constants sun.nio.cs.xxx.INSTANCE is not too bad even in the code that is unlikely to be executed during the VM startup. One small advantage is that if the code is copy/pasted within the java.base module, it will not bring the risk of early initialization of StandardCharsets. With NTLM, however, switching to StandardCharsets allows to remove sun.nio.cs.UTF_16LE.INSTANCE and all other corresponding modifications. So, I used StandardCharsets in NTLM (and in XML and SOCKS, as you suggested), and left sun.nio.cs constants in all other places. Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8234147/02/webrev/ It builds fine, tests run fine. Okay although my personal view is that a lot of this is unnecessary, e.g. the pack200 Driver class, IllegalAccessMap is not used at run-time with images builds, the InetAddress hosts file requires a special property to enable, and several others. On the other hand, there are several places where the handling of UnsupportedEncodingException goes away so those parts are good. -Alan
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
Hi Ulf! Thank you for the links! The enhancement requests that you pointed out are about making the lookup faster. My current proposal, however, is to completely avoid the lookup of the standard charsets in certain specific cases. With kind regards, Ivan On 11/30/19 1:32 PM, Ulf Zibis wrote: Hi Ivan, you may compare your proposal with: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6790402 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6850361 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6796087 -Ulf Am 27.11.19 um 05:39 schrieb Ivan Gerasimov: Hello! It is a cleanup fix with mostly two kinds of changes: - when a standard charset is specified by its name, use a preinitialized Charset constant instead, - replace the usage of StandardCharset.* constants with their sun.nio.cs.* equivalents to avoid accidental early initialization of rarely-used charsets. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8234147 WEBREV: http://cr.openjdk.java.net/~igerasim/8234147/00/webrev/ I also had to modify one regression test that relied on a private auxiliary method, which was removed with the fix. Mach5 control build looks green. Would you please help review the fix? -- With kind regards, Ivan Gerasimov
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
Thank you Alan for the further review! On 11/27/19 12:53 PM, Alan Bateman wrote: On 27/11/2019 11:52, Ivan Gerasimov wrote: : It's not clear how to distinguish the classes inside the java.base module that do not have to depend on sun.nio.cs. If you feel strong about NTLM and XML, I can replace sun.nio.cs.* instances with corresponding java.charset.StandardCharsets.* constants in these classes. There isn't any way to say what is core and non-core in java.base so you have to use your judgement. My personal view is that the NTLM, XML, SOCKS and several others in this patch should stick to the standard APIs if possible as their performance will likely be dominated by other factors. Personally, I think that using constants sun.nio.cs.xxx.INSTANCE is not too bad even in the code that is unlikely to be executed during the VM startup. One small advantage is that if the code is copy/pasted within the java.base module, it will not bring the risk of early initialization of StandardCharsets. With NTLM, however, switching to StandardCharsets allows to remove sun.nio.cs.UTF_16LE.INSTANCE and all other corresponding modifications. So, I used StandardCharsets in NTLM (and in XML and SOCKS, as you suggested), and left sun.nio.cs constants in all other places. Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8234147/02/webrev/ It builds fine, tests run fine. -- With kind regards, Ivan Gerasimov
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
Hi Ivan, you may compare your proposal with: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6790402 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6850361 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6796087 -Ulf Am 27.11.19 um 05:39 schrieb Ivan Gerasimov: Hello! It is a cleanup fix with mostly two kinds of changes: - when a standard charset is specified by its name, use a preinitialized Charset constant instead, - replace the usage of StandardCharset.* constants with their sun.nio.cs.* equivalents to avoid accidental early initialization of rarely-used charsets. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8234147 WEBREV: http://cr.openjdk.java.net/~igerasim/8234147/00/webrev/ I also had to modify one regression test that relied on a private auxiliary method, which was removed with the fix. Mach5 control build looks green. Would you please help review the fix?
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
On 27/11/2019 11:52, Ivan Gerasimov wrote: : It's not clear how to distinguish the classes inside the java.base module that do not have to depend on sun.nio.cs. If you feel strong about NTLM and XML, I can replace sun.nio.cs.* instances with corresponding java.charset.StandardCharsets.* constants in these classes. There isn't any way to say what is core and non-core in java.base so you have to use your judgement. My personal view is that the NTLM, XML, SOCKS and several others in this patch should stick to the standard APIs if possible as their performance will likely be dominated by other factors. Thanks, done. Unfortunately, imports conventions are not very consistent across the JDK code, so I mostly tried to preserve the pre-existing order of imports in each file. Okay. If I got it correct, the test was meant to access a private static method, and after removing Perf.getBytes there were no such methods left in the Perf class. That's why I had to pick some other method to test, so I chose ModulePath.packageName instead. The test is trying to cover all the possible accessibilities. The "exported package" descriptions are a bit confusing but are explained by the test running--add-exports. My only concern with the change is that it expands the dependences on internals to classes in 3 packages, it means anyone touching those classes may have to fix up this test. So overall I think this patch is okay, just me trying to avoid coupling when we can. -Alan
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
Thank you Alan for reviewing! Please see the comments inline. On 11/26/19 11:54 PM, Alan Bateman wrote: On 27/11/2019 04:39, Ivan Gerasimov wrote: Hello! It is a cleanup fix with mostly two kinds of changes: - when a standard charset is specified by its name, use a preinitialized Charset constant instead, - replace the usage of StandardCharset.* constants with their sun.nio.cs.* equivalents to avoid accidental early initialization of rarely-used charsets. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8234147 WEBREV: http://cr.openjdk.java.net/~igerasim/8234147/00/webrev/ I also had to modify one regression test that relied on a private auxiliary method, which was removed with the fix. Mach5 control build looks green. Would you please help review the fix? The motivation is okay and most of the changes are okay but it does create a needless dependency on sun.nio.* classes in places that are unlikely (or impossible) to use during early startup. I think the change to the JDBC CachedRow implementation, prefs, and the jdk.net.httpserver module should be dropped as want fewer classes outside of java.base depending on java.base internals, not more. I also wonder about the NTLM, XML properties and a few more that shouldn't be dependency on sun.nio.* classes if possible. Please note that in fact sun.nio.cs.* was not used in java.net.http, java.prefs and java.sql.rowset. I only used those sun.nio constants inside the java.base module, exactly for the reason of avoiding additional dependencies. It's not clear how to distinguish the classes inside the java.base module that do not have to depend on sun.nio.cs. If you feel strong about NTLM and XML, I can replace sun.nio.cs.* instances with corresponding java.charset.StandardCharsets.* constants in these classes. Minot nit but if this is pushed then it would be good to keep the imports consistent with the existing style where you can, e.g. I see several files where the import sun.nio.* is put at the top of source files that already group imports of JDK internal classes together further down. Thanks, done. Unfortunately, imports conventions are not very consistent across the JDK code, so I mostly tried to preserve the pre-existing order of imports in each file. Mandy might want to comment on TrySetAccessibleTest. If I'm not mistaken it should say "non-exported" rather than "exported". Also I think we want this test to have a dependency on a few JDK internal and private methods as possible as it's a maintenance issue to keep it up to date (as you've experienced here with the removal of Perf.getBytes). If I got it correct, the test was meant to access a private static method, and after removing Perf.getBytes there were no such methods left in the Perf class. That's why I had to pick some other method to test, so I chose ModulePath.packageName instead. Here's the updated webrev with reorganized imports: http://cr.openjdk.java.net/~igerasim/8234147/01/webrev/ Thanks again! -- With kind regards, Ivan Gerasimov
Re: RFR 8234147 : Avoid looking up standard charsets in core libraries
On 27/11/2019 04:39, Ivan Gerasimov wrote: Hello! It is a cleanup fix with mostly two kinds of changes: - when a standard charset is specified by its name, use a preinitialized Charset constant instead, - replace the usage of StandardCharset.* constants with their sun.nio.cs.* equivalents to avoid accidental early initialization of rarely-used charsets. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8234147 WEBREV: http://cr.openjdk.java.net/~igerasim/8234147/00/webrev/ I also had to modify one regression test that relied on a private auxiliary method, which was removed with the fix. Mach5 control build looks green. Would you please help review the fix? The motivation is okay and most of the changes are okay but it does create a needless dependency on sun.nio.* classes in places that are unlikely (or impossible) to use during early startup. I think the change to the JDBC CachedRow implementation, prefs, and the jdk.net.httpserver module should be dropped as want fewer classes outside of java.base depending on java.base internals, not more. I also wonder about the NTLM, XML properties and a few more that shouldn't be dependency on sun.nio.* classes if possible. Minot nit but if this is pushed then it would be good to keep the imports consistent with the existing style where you can, e.g. I see several files where the import sun.nio.* is put at the top of source files that already group imports of JDK internal classes together further down. Mandy might want to comment on TrySetAccessibleTest. If I'm not mistaken it should say "non-exported" rather than "exported". Also I think we want this test to have a dependency on a few JDK internal and private methods as possible as it's a maintenance issue to keep it up to date (as you've experienced here with the removal of Perf.getBytes). -Alan
