From 3b589c8f90775729896c40072cadd53488c598b5 Mon Sep 17 00:00:00 2001 From: felkachang Date: Sat, 3 Sep 2022 11:01:19 +0800 Subject: [PATCH] 10% improvement by changing FastNative to CriticalNative This patch change native APIs from @FastNative to @CriticalNative. From the benchmark, it gets 10% improvement. The CriticalNative requires the native function can't access JNIEnv and any jobject. nativeGetAttributeIndex can't apply CriticalNative because it needs JNIEnv to transform Java String. This patches move the 16 APIs from FastNative to CriticalNative. * 11 APIs check the null document and throw NPE in native layer. They are change to return ERROR_NULL_DOCUMENT and throw NPE in java layer. * 5 APIs just remove JNIEnv and jobject parameters. * nativeGetNamespace * nativeGetName * nativeGetText * nativeGetStyleAttribute * nativeGetSourceResId Reference: ag/19992907 Bug: 173709508 Test: # for FastNative atest --iterations 20 \ CorePerfTests:android.content.res.XmlBlockBenchmark \ 2>&1 > fn20.txt Test: # for CriticalNative atest --iterations 20 \ CorePerfTests:android.content.res.XmlBlockBenchmark \ 2>&1 > cn20.txt Test: http://b/issues/173709508#comment2 Change-Id: I43ccd1177fa86df5cd4d432119cbd06d370e82d8 --- core/java/android/content/res/XmlBlock.java | 203 +++++++++++++++----- core/jni/android_util_XmlBlock.cpp | 113 ++++------- 2 files changed, 197 insertions(+), 119 deletions(-) diff --git a/core/java/android/content/res/XmlBlock.java b/core/java/android/content/res/XmlBlock.java index 53dfed374fbca..3915a6ccb46df 100644 --- a/core/java/android/content/res/XmlBlock.java +++ b/core/java/android/content/res/XmlBlock.java @@ -17,6 +17,7 @@ package android.content.res; import static android.content.res.Resources.ID_NULL; +import static android.system.OsConstants.EINVAL; import android.annotation.AnyRes; import android.annotation.NonNull; @@ -28,6 +29,7 @@ import android.util.TypedValue; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.XmlUtils; +import dalvik.annotation.optimization.CriticalNative; import dalvik.annotation.optimization.FastNative; import org.xmlpull.v1.XmlPullParserException; @@ -92,6 +94,16 @@ public final class XmlBlock implements AutoCloseable { } } + /** + * Reference Error.h UNEXPECTED_NULL + */ + private static final int ERROR_NULL_DOCUMENT = Integer.MIN_VALUE + 8; + /** + * The reason not to ResXMLParser::BAD_DOCUMENT which is -1 is that other places use the same + * value. Reference Error.h BAD_VALUE = -EINVAL + */ + private static final int ERROR_BAD_DOCUMENT = -EINVAL; + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) public final class Parser implements XmlResourceParser { Parser(long parseState, XmlBlock block) { @@ -168,7 +180,11 @@ public final class XmlBlock implements AutoCloseable { return id >= 0 ? getSequenceString(mStrings.getSequence(id)) : null; } public int getLineNumber() { - return nativeGetLineNumber(mParseState); + final int lineNumber = nativeGetLineNumber(mParseState); + if (lineNumber == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return lineNumber; } public int getEventType() throws XmlPullParserException { return mEventType; @@ -203,7 +219,10 @@ public final class XmlBlock implements AutoCloseable { } @NonNull public String getAttributeNamespace(int index) { - int id = nativeGetAttributeNamespace(mParseState, index); + final int id = nativeGetAttributeNamespace(mParseState, index); + if (id == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } if (DEBUG) System.out.println("getAttributeNamespace of " + index + " = " + id); if (id >= 0) return getSequenceString(mStrings.getSequence(id)); else if (id == -1) return ""; @@ -211,8 +230,11 @@ public final class XmlBlock implements AutoCloseable { } @NonNull public String getAttributeName(int index) { - int id = nativeGetAttributeName(mParseState, index); + final int id = nativeGetAttributeName(mParseState, index); if (DEBUG) System.out.println("getAttributeName of " + index + " = " + id); + if (id == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } if (id >= 0) return getSequenceString(mStrings.getSequence(id)); throw new IndexOutOfBoundsException(String.valueOf(index)); } @@ -224,21 +246,38 @@ public final class XmlBlock implements AutoCloseable { return false; } public int getAttributeCount() { - return mEventType == START_TAG ? nativeGetAttributeCount(mParseState) : -1; + if (mEventType == START_TAG) { + final int count = nativeGetAttributeCount(mParseState); + if (count == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return count; + } else { + return -1; + } } @NonNull public String getAttributeValue(int index) { - int id = nativeGetAttributeStringValue(mParseState, index); + final int id = nativeGetAttributeStringValue(mParseState, index); + if (id == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } if (DEBUG) System.out.println("getAttributeValue of " + index + " = " + id); if (id >= 0) return getSequenceString(mStrings.getSequence(id)); // May be some other type... check and try to convert if so. - int t = nativeGetAttributeDataType(mParseState, index); + final int t = nativeGetAttributeDataType(mParseState, index); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } if (t == TypedValue.TYPE_NULL) { throw new IndexOutOfBoundsException(String.valueOf(index)); } - int v = nativeGetAttributeData(mParseState, index); + final int v = nativeGetAttributeData(mParseState, index); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } return TypedValue.coerceToString(t, v); } public String getAttributeType(int index) { @@ -272,6 +311,9 @@ public final class XmlBlock implements AutoCloseable { return END_DOCUMENT; } int ev = nativeNext(mParseState); + if (ev == ERROR_BAD_DOCUMENT) { + throw new XmlPullParserException("Corrupt XML binary file"); + } if (mDecNextDepth) { mDepth--; mDecNextDepth = false; @@ -338,7 +380,11 @@ public final class XmlBlock implements AutoCloseable { } public int getAttributeNameResource(int index) { - return nativeGetAttributeResource(mParseState, index); + final int resourceNameId = nativeGetAttributeResource(mParseState, index); + if (resourceNameId == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return resourceNameId; } public int getAttributeListValue(String namespace, String attribute, @@ -393,8 +439,14 @@ public final class XmlBlock implements AutoCloseable { public int getAttributeListValue(int idx, String[] options, int defaultValue) { - int t = nativeGetAttributeDataType(mParseState, idx); - int v = nativeGetAttributeData(mParseState, idx); + final int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } if (t == TypedValue.TYPE_STRING) { return XmlUtils.convertValueToList( mStrings.getSequence(v), options, defaultValue); @@ -403,62 +455,99 @@ public final class XmlBlock implements AutoCloseable { } public boolean getAttributeBooleanValue(int idx, boolean defaultValue) { - int t = nativeGetAttributeDataType(mParseState, idx); + final int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } // Note: don't attempt to convert any other types, because // we want to count on aapt doing the conversion for us. - if (t >= TypedValue.TYPE_FIRST_INT && - t <= TypedValue.TYPE_LAST_INT) { - return nativeGetAttributeData(mParseState, idx) != 0; + if (t >= TypedValue.TYPE_FIRST_INT && t <= TypedValue.TYPE_LAST_INT) { + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return v != 0; } return defaultValue; } public int getAttributeResourceValue(int idx, int defaultValue) { - int t = nativeGetAttributeDataType(mParseState, idx); + final int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } // Note: don't attempt to convert any other types, because // we want to count on aapt doing the conversion for us. if (t == TypedValue.TYPE_REFERENCE) { - return nativeGetAttributeData(mParseState, idx); + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return v; } return defaultValue; } public int getAttributeIntValue(int idx, int defaultValue) { - int t = nativeGetAttributeDataType(mParseState, idx); + final int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } // Note: don't attempt to convert any other types, because // we want to count on aapt doing the conversion for us. - if (t >= TypedValue.TYPE_FIRST_INT && - t <= TypedValue.TYPE_LAST_INT) { - return nativeGetAttributeData(mParseState, idx); + if (t >= TypedValue.TYPE_FIRST_INT && t <= TypedValue.TYPE_LAST_INT) { + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return v; } return defaultValue; } public int getAttributeUnsignedIntValue(int idx, int defaultValue) { int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } // Note: don't attempt to convert any other types, because // we want to count on aapt doing the conversion for us. - if (t >= TypedValue.TYPE_FIRST_INT && - t <= TypedValue.TYPE_LAST_INT) { - return nativeGetAttributeData(mParseState, idx); + if (t >= TypedValue.TYPE_FIRST_INT && t <= TypedValue.TYPE_LAST_INT) { + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return v; } return defaultValue; } public float getAttributeFloatValue(int idx, float defaultValue) { - int t = nativeGetAttributeDataType(mParseState, idx); + final int t = nativeGetAttributeDataType(mParseState, idx); + if (t == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } // Note: don't attempt to convert any other types, because // we want to count on aapt doing the conversion for us. if (t == TypedValue.TYPE_FLOAT) { - return Float.intBitsToFloat( - nativeGetAttributeData(mParseState, idx)); + final int v = nativeGetAttributeData(mParseState, idx); + if (v == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return Float.intBitsToFloat(v); } throw new RuntimeException("not a float!"); } @Nullable public String getIdAttribute() { - int id = nativeGetIdAttribute(mParseState); + final int id = nativeGetIdAttribute(mParseState); + if (id == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } return id >= 0 ? getSequenceString(mStrings.getSequence(id)) : null; } @Nullable public String getClassAttribute() { - int id = nativeGetClassAttribute(mParseState); + final int id = nativeGetClassAttribute(mParseState); + if (id == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } return id >= 0 ? getSequenceString(mStrings.getSequence(id)) : null; } @@ -468,7 +557,11 @@ public final class XmlBlock implements AutoCloseable { } public int getStyleAttribute() { - return nativeGetStyleAttribute(mParseState); + final int styleAttributeId = nativeGetStyleAttribute(mParseState); + if (styleAttributeId == ERROR_NULL_DOCUMENT) { + throw new NullPointerException("Null document"); + } + return styleAttributeId; } private String getSequenceString(@Nullable CharSequence str) { @@ -544,37 +637,55 @@ public final class XmlBlock implements AutoCloseable { // ----------- @FastNative ------------------ @FastNative + private static native int nativeGetAttributeIndex( + long state, String namespace, String name); + + // ----------- @CriticalNative ------------------ + @CriticalNative /*package*/ static final native int nativeNext(long state); - @FastNative + + @CriticalNative private static final native int nativeGetNamespace(long state); - @FastNative + + @CriticalNative /*package*/ static final native int nativeGetName(long state); - @FastNative + + @CriticalNative private static final native int nativeGetText(long state); - @FastNative + + @CriticalNative private static final native int nativeGetLineNumber(long state); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeCount(long state); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeNamespace(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeName(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeResource(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeDataType(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeData(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetAttributeStringValue(long state, int idx); - @FastNative + + @CriticalNative private static final native int nativeGetIdAttribute(long state); - @FastNative + + @CriticalNative private static final native int nativeGetClassAttribute(long state); - @FastNative + + @CriticalNative private static final native int nativeGetStyleAttribute(long state); - @FastNative - private static final native int nativeGetAttributeIndex(long state, String namespace, String name); - @FastNative + + @CriticalNative private static final native int nativeGetSourceResId(long state); } diff --git a/core/jni/android_util_XmlBlock.cpp b/core/jni/android_util_XmlBlock.cpp index 891330082f3ea..5a444bb1d0ff3 100644 --- a/core/jni/android_util_XmlBlock.cpp +++ b/core/jni/android_util_XmlBlock.cpp @@ -28,6 +28,9 @@ #include namespace android { +constexpr int kNullDocument = UNEXPECTED_NULL; +// The reason not to ResXMLParser::BAD_DOCUMENT which is -1 is that other places use the same value. +constexpr int kBadDocument = BAD_VALUE; // ---------------------------------------------------------------------------- @@ -92,9 +95,7 @@ static jlong android_content_XmlBlock_nativeCreateParseState(JNIEnv* env, jobjec return reinterpret_cast(st); } -static jint android_content_XmlBlock_nativeNext(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeNext(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { return ResXMLParser::END_DOCUMENT; @@ -121,14 +122,10 @@ static jint android_content_XmlBlock_nativeNext(JNIEnv* env, jobject clazz, } while (true); bad: - jniThrowException(env, "org/xmlpull/v1/XmlPullParserException", - "Corrupt XML binary file"); - return ResXMLParser::BAD_DOCUMENT; + return kBadDocument; } -static jint android_content_XmlBlock_nativeGetNamespace(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetNamespace(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { return -1; @@ -137,9 +134,7 @@ static jint android_content_XmlBlock_nativeGetNamespace(JNIEnv* env, jobject cla return static_cast(st->getElementNamespaceID()); } -static jint android_content_XmlBlock_nativeGetName(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetName(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { return -1; @@ -148,9 +143,7 @@ static jint android_content_XmlBlock_nativeGetName(JNIEnv* env, jobject clazz, return static_cast(st->getElementNameID()); } -static jint android_content_XmlBlock_nativeGetText(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetText(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { return -1; @@ -159,97 +152,80 @@ static jint android_content_XmlBlock_nativeGetText(JNIEnv* env, jobject clazz, return static_cast(st->getTextID()); } -static jint android_content_XmlBlock_nativeGetLineNumber(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetLineNumber(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getLineNumber()); } -static jint android_content_XmlBlock_nativeGetAttributeCount(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetAttributeCount( + CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeCount()); } -static jint android_content_XmlBlock_nativeGetAttributeNamespace(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeNamespace( + CRITICAL_JNI_PARAMS_COMMA jlong token, jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeNamespaceID(idx)); } -static jint android_content_XmlBlock_nativeGetAttributeName(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeName(CRITICAL_JNI_PARAMS_COMMA jlong token, + jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeNameID(idx)); } -static jint android_content_XmlBlock_nativeGetAttributeResource(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeResource( + CRITICAL_JNI_PARAMS_COMMA jlong token, jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeNameResID(idx)); } -static jint android_content_XmlBlock_nativeGetAttributeDataType(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeDataType( + CRITICAL_JNI_PARAMS_COMMA jlong token, jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeDataType(idx)); } -static jint android_content_XmlBlock_nativeGetAttributeData(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeData(CRITICAL_JNI_PARAMS_COMMA jlong token, + jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeData(idx)); } -static jint android_content_XmlBlock_nativeGetAttributeStringValue(JNIEnv* env, jobject clazz, - jlong token, jint idx) -{ +static jint android_content_XmlBlock_nativeGetAttributeStringValue( + CRITICAL_JNI_PARAMS_COMMA jlong token, jint idx) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } return static_cast(st->getAttributeValueStringID(idx)); @@ -286,39 +262,32 @@ static jint android_content_XmlBlock_nativeGetAttributeIndex(JNIEnv* env, jobjec return idx; } -static jint android_content_XmlBlock_nativeGetIdAttribute(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetIdAttribute(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } ssize_t idx = st->indexOfID(); return idx >= 0 ? static_cast(st->getAttributeValueStringID(idx)) : -1; } -static jint android_content_XmlBlock_nativeGetClassAttribute(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetClassAttribute( + CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } ssize_t idx = st->indexOfClass(); return idx >= 0 ? static_cast(st->getAttributeValueStringID(idx)) : -1; } -static jint android_content_XmlBlock_nativeGetStyleAttribute(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetStyleAttribute( + CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { - jniThrowNullPointerException(env, NULL); - return 0; + return kNullDocument; } ssize_t idx = st->indexOfStyle(); @@ -336,9 +305,7 @@ static jint android_content_XmlBlock_nativeGetStyleAttribute(JNIEnv* env, jobjec ? value.data : 0; } -static jint android_content_XmlBlock_nativeGetSourceResId(JNIEnv* env, jobject clazz, - jlong token) -{ +static jint android_content_XmlBlock_nativeGetSourceResId(CRITICAL_JNI_PARAMS_COMMA jlong token) { ResXMLParser* st = reinterpret_cast(token); if (st == NULL) { return 0;