Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit eac0321f authored by TreeHugger Robot's avatar TreeHugger Robot Committed by Android (Google) Code Review
Browse files

Merge changes from topic "oct16b"

* changes:
  Recommend efficient String operations.
  Simple alternative to String.format().
parents fe82ad21 a52266b3
Loading
Loading
Loading
Loading
+109 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2020 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package android.text;

import android.perftests.utils.BenchmarkState;
import android.perftests.utils.PerfStatusReporter;

import androidx.test.filters.LargeTest;
import androidx.test.runner.AndroidJUnit4;

import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;

import java.util.function.Supplier;

@RunWith(AndroidJUnit4.class)
@LargeTest
public class TextUtilsPerfTest {
    @Rule
    public PerfStatusReporter mPerfStatusReporter = new PerfStatusReporter();

    public static final String TEMPLATE = "Template that combines %s and %d together";

    public String mVar1 = "example";
    public int mVar2 = 42;

    /**
     * Measure overhead of formatting a string via {@link String#format}.
     */
    @Test
    public void timeFormatUpstream() {
        final BenchmarkState state = mPerfStatusReporter.getBenchmarkState();
        while (state.keepRunning()) {
            String res = String.format(TEMPLATE, mVar1, mVar2);
        }
    }

    /**
     * Measure overhead of formatting a string via
     * {@link TextUtils#formatSimple}.
     */
    @Test
    public void timeFormatLocal() {
        final BenchmarkState state = mPerfStatusReporter.getBenchmarkState();
        while (state.keepRunning()) {
            String res = TextUtils.formatSimple(TEMPLATE, mVar1, mVar2);
        }
    }

    /**
     * Measure overhead of formatting a string inline.
     */
    @Test
    public void timeFormatInline() {
        final BenchmarkState state = mPerfStatusReporter.getBenchmarkState();
        while (state.keepRunning()) {
            String res = "Template that combines " + mVar1 + " and " + mVar2 + " together";
        }
    }

    /**
     * Measure overhead of a passing null-check that uses a lambda to
     * communicate a custom error message.
     */
    @Test
    public void timeFormat_Skip_Lambda() {
        final BenchmarkState state = mPerfStatusReporter.getBenchmarkState();
        while (state.keepRunning()) {
            requireNonNull(this, () -> {
                return String.format(TEMPLATE, mVar1, mVar2);
            });
        }
    }

    /**
     * Measure overhead of a passing null-check that uses varargs to communicate
     * a custom error message.
     */
    @Test
    public void timeFormat_Skip_Varargs() {
        final BenchmarkState state = mPerfStatusReporter.getBenchmarkState();
        while (state.keepRunning()) {
            requireNonNull(this, TEMPLATE, mVar1, mVar2);
        }
    }

    private static <T> T requireNonNull(T obj, Supplier<String> messageSupplier) {
        return obj;
    }

    private static <T> T requireNonNull(T obj, String format, Object... args) {
        return obj;
    }
}
+88 −0
Original line number Diff line number Diff line
@@ -2079,6 +2079,94 @@ public class TextUtils {
        return Resources.getSystem().getQuantityString(R.plurals.selected_count, count, count);
    }

    /**
     * Simple alternative to {@link String#format} which purposefully supports
     * only a small handful of substitutions to improve execution speed.
     * Benchmarking reveals this optimized alternative performs 6.5x faster for
     * a typical format string.
     * <p>
     * Below is a summary of the limited grammar supported by this method; if
     * you need advanced features, please continue using {@link String#format}.
     * <ul>
     * <li>{@code %b} for {@code boolean}
     * <li>{@code %c} for {@code char}
     * <li>{@code %d} for {@code int} or {@code long}
     * <li>{@code %f} for {@code float} or {@code double}
     * <li>{@code %s} for {@code String}
     * <li>{@code %x} for hex representation of {@code int} or {@code long}
     * <li>{@code %%} for literal {@code %}
     * </ul>
     *
     * @throws IllegalArgumentException if the format string or arguments don't
     *             match the supported grammar described above.
     * @hide
     */
    public static @NonNull String formatSimple(@NonNull String format, Object... args) {
        final StringBuilder sb = new StringBuilder(format);
        int j = 0;
        for (int i = 0; i < sb.length(); ) {
            if (sb.charAt(i) == '%') {
                final String repl;
                final char code = sb.charAt(i + 1);
                switch (code) {
                    case 'b': {
                        if (j == args.length) {
                            throw new IllegalArgumentException("Too few arguments");
                        }
                        final Object arg = args[j++];
                        if (arg instanceof Boolean) {
                            repl = Boolean.toString((boolean) arg);
                        } else {
                            repl = Boolean.toString(arg != null);
                        }
                        break;
                    }
                    case 'c':
                    case 'd':
                    case 'f':
                    case 's': {
                        if (j == args.length) {
                            throw new IllegalArgumentException("Too few arguments");
                        }
                        final Object arg = args[j++];
                        repl = String.valueOf(arg);
                        break;
                    }
                    case 'x': {
                        if (j == args.length) {
                            throw new IllegalArgumentException("Too few arguments");
                        }
                        final Object arg = args[j++];
                        if (arg instanceof Integer) {
                            repl = Integer.toHexString((int) arg);
                        } else if (arg instanceof Long) {
                            repl = Long.toHexString((long) arg);
                        } else {
                            throw new IllegalArgumentException(
                                    "Unsupported hex type " + arg.getClass());
                        }
                        break;
                    }
                    case '%': {
                        repl = "%";
                        break;
                    }
                    default: {
                        throw new IllegalArgumentException("Unsupported format code " + code);
                    }
                }
                sb.replace(i, i + 2, repl);
                i += repl.length();
            } else {
                i++;
            }
        }
        if (j != args.length) {
            throw new IllegalArgumentException("Too many arguments");
        }
        return sb.toString();
    }

    /**
     * Returns whether or not the specified spanned text has a style span.
     * @hide
+57 −9
Original line number Diff line number Diff line
@@ -31,6 +31,12 @@ import java.util.Objects;
 */
public class Preconditions {

    /**
     * Ensures that an expression checking an argument is true.
     *
     * @param expression the expression to check
     * @throws IllegalArgumentException if {@code expression} is false
     */
    @UnsupportedAppUsage
    public static void checkArgument(boolean expression) {
        if (!expression) {
@@ -62,8 +68,9 @@ public class Preconditions {
     * @param messageArgs arguments for {@code messageTemplate}
     * @throws IllegalArgumentException if {@code expression} is false
     */
    public static void checkArgument(boolean expression,
            final String messageTemplate,
    public static void checkArgument(
            final boolean expression,
            final @NonNull String messageTemplate,
            final Object... messageArgs) {
        if (!expression) {
            throw new IllegalArgumentException(String.format(messageTemplate, messageArgs));
@@ -114,7 +121,9 @@ public class Preconditions {
     * @throws IllegalArgumentException if {@code string} is empty
     */
    public static @NonNull <T extends CharSequence> T checkStringNotEmpty(
            final T string, final String messageTemplate, final Object... messageArgs) {
            final T string,
            final @NonNull String messageTemplate,
            final Object... messageArgs) {
        if (TextUtils.isEmpty(string)) {
            throw new IllegalArgumentException(String.format(messageTemplate, messageArgs));
        }
@@ -159,18 +168,50 @@ public class Preconditions {
        return reference;
    }

    /**
     * Ensures that an object reference passed as a parameter to the calling
     * method is not null.
     *
     * @param messageTemplate a printf-style message template to use if the check fails; will
     *     be converted to a string using {@link String#format(String, Object...)}
     * @param messageArgs arguments for {@code messageTemplate}
     * @throws NullPointerException if {@code reference} is null
     */
    public static @NonNull <T> T checkNotNull(
            final T reference,
            final @NonNull String messageTemplate,
            final Object... messageArgs) {
        if (reference == null) {
            throw new NullPointerException(String.format(messageTemplate, messageArgs));
        }
        return reference;
    }

    /**
     * Ensures the truth of an expression involving the state of the calling
     * instance, but not involving any parameters to the calling method.
     *
     * @param expression a boolean expression
     * @param message exception message
     * @throws IllegalStateException if {@code expression} is false
     */
    @UnsupportedAppUsage
    public static void checkState(final boolean expression, String message) {
    public static void checkState(final boolean expression) {
        checkState(expression, null);
    }

    /**
     * Ensures the truth of an expression involving the state of the calling
     * instance, but not involving any parameters to the calling method.
     *
     * @param expression a boolean expression
     * @param errorMessage the exception message to use if the check fails; will
     *     be converted to a string using {@link String#valueOf(Object)}
     * @throws IllegalStateException if {@code expression} is false
     */
    @UnsupportedAppUsage
    public static void checkState(final boolean expression, String errorMessage) {
        if (!expression) {
            throw new IllegalStateException(message);
            throw new IllegalStateException(errorMessage);
        }
    }

@@ -179,11 +220,18 @@ public class Preconditions {
     * instance, but not involving any parameters to the calling method.
     *
     * @param expression a boolean expression
     * @param messageTemplate a printf-style message template to use if the check fails; will
     *     be converted to a string using {@link String#format(String, Object...)}
     * @param messageArgs arguments for {@code messageTemplate}
     * @throws IllegalStateException if {@code expression} is false
     */
    @UnsupportedAppUsage
    public static void checkState(final boolean expression) {
        checkState(expression, null);
    public static void checkState(
            final boolean expression,
            final @NonNull String messageTemplate,
            final Object... messageArgs) {
        if (!expression) {
            throw new IllegalStateException(String.format(messageTemplate, messageArgs));
        }
    }

    /**
+51 −0
Original line number Diff line number Diff line
@@ -16,6 +16,8 @@

package android.text;

import static android.text.TextUtils.formatSimple;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
@@ -794,4 +796,53 @@ public class TextUtilsTest {
        assertEquals("", TextUtils.trimToLengthWithEllipsis("", 3));
        assertNull(TextUtils.trimToLengthWithEllipsis(null, 3));
    }

    @Test
    public void testFormatSimple_Types() {
        assertEquals("true", formatSimple("%b", true));
        assertEquals("false", formatSimple("%b", false));
        assertEquals("true", formatSimple("%b", this));
        assertEquals("false", formatSimple("%b", new Object[] { null }));

        assertEquals("!", formatSimple("%c", '!'));

        assertEquals("42", formatSimple("%d", 42));
        assertEquals("281474976710656", formatSimple("%d", 281474976710656L));

        assertEquals("3.14159", formatSimple("%f", 3.14159));
        assertEquals("NaN", formatSimple("%f", Float.NaN));

        assertEquals("example", formatSimple("%s", "example"));
        assertEquals("null", formatSimple("%s", new Object[] { null }));

        assertEquals("2a", formatSimple("%x", 42));
        assertEquals("1000000000000", formatSimple("%x", 281474976710656L));

        assertEquals("%", formatSimple("%%"));
    }

    @Test
    public void testFormatSimple_Empty() {
        assertEquals("", formatSimple(""));
    }

    @Test
    public void testFormatSimple_Typical() {
        assertEquals("String foobar and %% number -42 together",
                formatSimple("String %s%s and %%%% number %d%d together", "foo", "bar", -4, 2));
    }

    @Test
    public void testFormatSimple_Mismatch() {
        try {
            formatSimple("%s");
            fail();
        } catch (IllegalArgumentException expected) {
        }
        try {
            formatSimple("%s", "foo", "bar");
            fail();
        } catch (IllegalArgumentException expected) {
        }
    }
}
+146 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2020 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.google.errorprone.bugpatterns.android;

import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.contains;
import static com.google.errorprone.matchers.Matchers.kindIs;
import static com.google.errorprone.matchers.Matchers.methodInvocation;
import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.matchers.Matchers.staticMethod;

import com.google.auto.service.AutoService;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.predicates.TypePredicate;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;

import java.util.List;

/**
 * Android offers several efficient alternatives to some upstream {@link String}
 * operations.
 */
@AutoService(BugChecker.class)
@BugPattern(
    name = "AndroidFrameworkEfficientStrings",
    summary = "Verifies efficient Strings best-practices",
    severity = WARNING)
public final class EfficientStringsChecker extends BugChecker
        implements MethodInvocationTreeMatcher {

    private static final Matcher<ExpressionTree> FORMAT_CALL = methodInvocation(
            staticMethod().onClass("java.lang.String").named("format"));
    private static final Matcher<ExpressionTree> PRECONDITIONS_CALL = methodInvocation(
            staticMethod().onClass(withSimpleName("Preconditions")).withAnyName());
    private static final Matcher<ExpressionTree> OBJECTS_CALL = methodInvocation(
            staticMethod().onClass("java.util.Objects").named("requireNonNull"));

    /**
     * Match an expression which combines both string literals any other dynamic
     * values, since these allocate a transparent StringBuilder.
     * <p>
     * This won't match a single isolated string literal, or a chain consisting
     * of only string literals, since those don't require dynamic construction.
     */
    private static final Matcher<ExpressionTree> CONTAINS_DYNAMIC_STRING = allOf(
            contains(ExpressionTree.class, kindIs(Kind.STRING_LITERAL)),
            contains(ExpressionTree.class, not(kindIs(Kind.STRING_LITERAL))));

    @Override
    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
        if (FORMAT_CALL.matches(tree, state)) {
            // Skip over possible locale to find format string
            final List<? extends ExpressionTree> args = tree.getArguments();
            final ExpressionTree formatArg;
            final List<VarSymbol> vars = ASTHelpers.getSymbol(tree).params();
            if (vars.get(0).type.toString().equals("java.util.Locale")) {
                formatArg = args.get(1);
            } else {
                formatArg = args.get(0);
            }

            // Determine if format string is "simple" enough to replace
            if (formatArg.getKind() == Kind.STRING_LITERAL) {
                final String format = String.valueOf(((LiteralTree) formatArg).getValue());
                if (isSimple(format)) {
                    return buildDescription(formatArg)
                            .setMessage("Simple format strings can be replaced with "
                                    + "TextUtils.formatSimple() for a 6x performance improvement")
                            .build();
                }
            }
        } else if (PRECONDITIONS_CALL.matches(tree, state)
                || OBJECTS_CALL.matches(tree, state)) {
            final List<? extends ExpressionTree> args = tree.getArguments();
            for (int i = 1 ; i < args.size(); i++) {
                final ExpressionTree arg = args.get(i);
                if (CONTAINS_DYNAMIC_STRING.matches(arg, state)) {
                    return buildDescription(arg)
                            .setMessage("Building dynamic messages is discouraged, since they "
                                    + "always allocate a transparent StringBuilder, even in "
                                    + "the successful case")
                            .build();
                }
            }
        }
        return Description.NO_MATCH;
    }

    static boolean isSimple(String format) {
        for (int i = 0; i < format.length(); i++) {
            char c = format.charAt(i);
            if (c == '%') {
                i++;
                c = format.charAt(i);
                switch (c) {
                    case 'b':
                    case 'c':
                    case 'd':
                    case 'f':
                    case 's':
                    case 'x':
                    case '%':
                        break;
                    default:
                        return false;
                }
            }
        }
        return true;
    }

    static TypePredicate withSimpleName(final String filter) {
        return new TypePredicate() {
            @Override
            public boolean apply(Type type, VisitorState state) {
                return type.tsym.getSimpleName().toString().equals(filter);
            }
        };
    }
}
Loading