On Sun, 17 Jan 2021 09:21:27 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> This looks much better now. Maybe just one small suggestion:
>> `java.lang.StringCoding#lookupCharset` is used in two places and in both
>> places the same exception handling/rethrowing logic is wrapped around the
>> invocation. So you could move that logic into the lookupCharset method which
>> would simplify call sites. You could even get rid of String.lookupCharset
>> method that way.
>
> When you combine the logic of String.lookupCharset:
>
> private static Charset lookupCharset(String charsetName)
> throws UnsupportedEncodingException {
> Objects.requireNonNull(charsetName);
> try {
> Charset cs = StringCoding.lookupCharset(charsetName);
> if (cs == null) {
> throw new UnsupportedEncodingException(charsetName);
> }
> return cs;
> } catch (IllegalCharsetNameException ics) {
> throw new UnsupportedEncodingException(charsetName);
> }
> }
>
> ... and StringCoding.lookupCharset:
>
> static Charset lookupCharset(String csn) {
> if (Charset.isSupported(csn)) {
> try {
> return Charset.forName(csn);
> } catch (UnsupportedCharsetException x) {
> throw new Error(x);
> }
> }
> return null;
> }
>
>
> ...you get this:
>
> private static Charset lookupCharset(String charsetName)
> throws UnsupportedEncodingException {
> Objects.requireNonNull(charsetName);
> try {
> Charset cs;
> inner: {
> if (Charset.isSupported(charsetName)) {
> try {
> cs = Charset.forName(charsetName);
> break inner;
> } catch (UnsupportedCharsetException x) {
> throw new Error(x);
> }
> }
> cs = null;
> }
> if (cs == null) {
> throw new UnsupportedEncodingException(charsetName);
> }
> return cs;
> } catch (IllegalCharsetNameException ics) {
> throw new UnsupportedEncodingException(charsetName);
> }
> }
>
>
> ...and that can be simplified to this:
>
> static Charset lookupCharset(String csn) throws
> UnsupportedEncodingException {
> Objects.requireNonNull(csn);
> try {
> return Charset.forName(csn);
> } catch (UnsupportedCharsetException | IllegalCharsetNameException x)
> {
> throw new UnsupportedEncodingException(csn);
> }
> }
>
> which has an additional benefit that it only performs one lookup
> (Charset.forName) instead of two (Charset.isSupported & Charset.forName)...
@plevart: I simplified the lookup logic based on your suggestion. Removed some
unreachable paths in and simplified `StringCoding.encode(String, byte,
byte[])` as a result.
Simplifying to one lookup speeds up `decodeCharsetName` cases a notch:
before:
decodeCharsetName UTF-8 avgt 15 370.337 ± 22.199 ns/op
after:
decodeCharsetName UTF-8 avgt 15 335.971 ± 15.894 ns/op
-------------
PR: https://git.openjdk.java.net/jdk/pull/2102
