RFR: 8259842: Remove Result cache from StringCoding [v4]

[Peter Levart]

On Sun, 17 Jan 2021 09:03:30 GMT, Peter Levart <plevart at openjdk.org> wrote:

>>> Do you think this code belongs more to String than to StringCoding?
>> 
>> I consider StringCoding an implementation detail of String, so I'm not sure there's (much) value in maintaining the separation of concern if it is cause for a performance loss. While encapsulating and separating concerns is a fine purpose I think the main purpose served by StringCoding is to resolve some bootstrap issues: String is one of the first classes to be loaded and eagerly pulling in dependencies like ThreadLocal and Charsets before bootstrapping leads to all manner of hard to decipher issues (yes - I've tried :-)).
>
> 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)...

-------------

PR: https://git.openjdk.java.net/jdk/pull/2102