RFR 8234147 : Avoid looking up standard charsets in core libraries

[Ivan Gerasimov]

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