From abe50314163dcc8faf05cf21d649899146bbdbdc Mon Sep 17 00:00:00 2001 From: Seigo Nonaka Date: Tue, 20 Feb 2018 14:49:59 -0800 Subject: [PATCH] Stop using finalizer in Typeface and FontFamily Use NativeAllocationRegistry instead. Verified this doesn't cause performance regressions: StaticLayout creation time: MeasuredText Balanced Hyphenation : 721,055 -> 723,812: (+0.4%) MeasuredText Balanced NoHyphenation: 527,880 -> 505,843: (-4.2%) MeasuredText Greedy Hyphenation : 476,982 -> 460,241: (-3.5%) MeasuredText Greedy NoHyphenation : 471,074 -> 459,013: (-2.6%) RandomText Balanced Hyphenation : 18,566,915 -> 18,514,633: (-0.3%) RandomText Balanced NoHyphenation : 7,697,772 -> 7,658,458: (-0.5%) RandomText Greedy Hyphenation : 7,648,606 -> 7,602,026: (-0.6%) RandomText Greedy NoHyphenation : 7,643,946 -> 7,618,898: (-0.3%) MeasuredText creation time: NoStyled Hyphenation : 18,377,293 -> 18,521,296: (+0.8%) NoStyled Hyphenation WidthOnly : 17,852,099 -> 17,889,073: (+0.2%) NoStyled NoHyphenation : 7,608,972 -> 7,614,311: (+0.1%) NoStyled NoHyphenation WidthOnly : 7,194,461 -> 7,192,043: (-0.0%) Styled Hyphenation : 15,396,698 -> 15,352,501: (-0.3%) Styled Hyphenation WidthOnly : 14,356,020 -> 14,379,205: (+0.2%) Styled NoHyphenation : 14,926,657 -> 14,971,723: (+0.3%) Styled NoHyphenation WidthOnly : 13,964,537 -> 13,874,324: (-0.6%) StaticLayout draw time: MeasuredText NoStyled : 644,795 -> 635,241: (-1.5%) MeasuredText NoStyled WithoutCache : 637,943 -> 616,369: (-3.4%) MeasuredText Styled : 866,664 -> 860,322: (-0.7%) MeasuredText Styled WithoutCache : 917,795 -> 897,164: (-2.2%) RandomText NoStyled : 538,196 -> 533,253: (-0.9%) RandomText NoStyled WithoutCache : 7,012,076 -> 7,047,498: (+0.5%) RandomText Styled : 3,011,941 -> 2,999,661: (-0.4%) RandomText Styled WithoutCache : 3,486,225 -> 3,462,493: (-0.7%) Bug: 70185027 Test: atest CtsWidgetTestCases:EditTextTest CtsWidgetTestCases:TextViewFadingEdgeTest FrameworksCoreTests:TextViewFallbackLineSpacingTest FrameworksCoreTests:TextViewTest FrameworksCoreTests:TypefaceTest CtsGraphicsTestCases:TypefaceTest CtsWidgetTestCases:TextViewTest CtsTextTestCases Change-Id: I7ebb8a219a8eb07c8ad9fa432e8454e4e06be386 --- core/jni/android/graphics/FontFamily.cpp | 56 ++++++++++++++++--------- core/jni/android/graphics/Typeface.cpp | 58 ++++++++++++++------------ graphics/java/android/graphics/FontFamily.java | 36 ++++++++-------- graphics/java/android/graphics/Typeface.java | 34 ++++++++------- 4 files changed, 107 insertions(+), 77 deletions(-) diff --git a/core/jni/android/graphics/FontFamily.cpp b/core/jni/android/graphics/FontFamily.cpp index ed032c78f6c7..c7ad2a47c367 100644 --- a/core/jni/android/graphics/FontFamily.cpp +++ b/core/jni/android/graphics/FontFamily.cpp @@ -51,6 +51,18 @@ struct NativeFamilyBuilder { std::vector axes; }; +static inline NativeFamilyBuilder* toNativeBuilder(jlong ptr) { + return reinterpret_cast(ptr); +} + +static inline FontFamilyWrapper* toFamily(jlong ptr) { + return reinterpret_cast(ptr); +} + +template static inline jlong toJLong(Ptr ptr) { + return reinterpret_cast(ptr); +} + static jlong FontFamily_initBuilder(JNIEnv* env, jobject clazz, jstring langs, jint variant) { NativeFamilyBuilder* builder; if (langs != nullptr) { @@ -59,15 +71,14 @@ static jlong FontFamily_initBuilder(JNIEnv* env, jobject clazz, jstring langs, j } else { builder = new NativeFamilyBuilder(minikin::registerLocaleList(""), variant); } - return reinterpret_cast(builder); + return toJLong(builder); } static jlong FontFamily_create(jlong builderPtr) { if (builderPtr == 0) { return 0; } - std::unique_ptr builder( - reinterpret_cast(builderPtr)); + NativeFamilyBuilder* builder = toNativeBuilder(builderPtr); if (builder->fonts.empty()) { return 0; } @@ -76,17 +87,23 @@ static jlong FontFamily_create(jlong builderPtr) { if (family->getCoverage().length() == 0) { return 0; } - return reinterpret_cast(new FontFamilyWrapper(std::move(family))); + return toJLong(new FontFamilyWrapper(std::move(family))); } -static void FontFamily_abort(jlong builderPtr) { - NativeFamilyBuilder* builder = reinterpret_cast(builderPtr); - delete builder; +static void releaseBuilder(jlong builderPtr) { + delete toNativeBuilder(builderPtr); } -static void FontFamily_unref(jlong familyPtr) { - FontFamilyWrapper* family = reinterpret_cast(familyPtr); - delete family; +static jlong FontFamily_getBuilderReleaseFunc() { + return toJLong(&releaseBuilder); +} + +static void releaseFamily(jlong familyPtr) { + delete toFamily(familyPtr); +} + +static jlong FontFamily_getFamilyReleaseFunc() { + return toJLong(&releaseFamily); } static bool addSkTypeface(NativeFamilyBuilder* builder, sk_sp&& data, int ttcIndex, @@ -175,7 +192,7 @@ static jboolean FontFamily_addFont(JNIEnv* env, jobject clazz, jlong builderPtr, static jboolean FontFamily_addFontWeightStyle(JNIEnv* env, jobject clazz, jlong builderPtr, jobject font, jint ttcIndex, jint weight, jint isItalic) { NPE_CHECK_RETURN_ZERO(env, font); - NativeFamilyBuilder* builder = reinterpret_cast(builderPtr); + NativeFamilyBuilder* builder = toNativeBuilder(builderPtr); const void* fontPtr = env->GetDirectBufferAddress(font); if (fontPtr == NULL) { ALOGE("addFont failed to create font, buffer invalid"); @@ -204,8 +221,7 @@ static jboolean FontFamily_addFontFromAssetManager(JNIEnv* env, jobject, jlong b NPE_CHECK_RETURN_ZERO(env, jassetMgr); NPE_CHECK_RETURN_ZERO(env, jpath); - NativeFamilyBuilder* builder = reinterpret_cast(builderPtr); - + NativeFamilyBuilder* builder = toNativeBuilder(builderPtr); Guarded* mgr = AssetManagerForJavaObject(env, jassetMgr); if (NULL == mgr) { builder->axes.clear(); @@ -249,19 +265,19 @@ static jboolean FontFamily_addFontFromAssetManager(JNIEnv* env, jobject, jlong b } static void FontFamily_addAxisValue(jlong builderPtr, jint tag, jfloat value) { - NativeFamilyBuilder* builder = reinterpret_cast(builderPtr); + NativeFamilyBuilder* builder = toNativeBuilder(builderPtr); builder->axes.push_back({static_cast(tag), value}); } /////////////////////////////////////////////////////////////////////////////// static const JNINativeMethod gFontFamilyMethods[] = { - { "nInitBuilder", "(Ljava/lang/String;I)J", (void*)FontFamily_initBuilder }, - { "nCreateFamily", "(J)J", (void*)FontFamily_create }, - { "nAbort", "(J)V", (void*)FontFamily_abort }, - { "nUnrefFamily", "(J)V", (void*)FontFamily_unref }, - { "nAddFont", "(JLjava/nio/ByteBuffer;III)Z", (void*)FontFamily_addFont }, - { "nAddFontWeightStyle", "(JLjava/nio/ByteBuffer;III)Z", + { "nInitBuilder", "(Ljava/lang/String;I)J", (void*)FontFamily_initBuilder }, + { "nCreateFamily", "(J)J", (void*)FontFamily_create }, + { "nGetBuilderReleaseFunc", "()J", (void*)FontFamily_getBuilderReleaseFunc }, + { "nGetFamilyReleaseFunc", "()J", (void*)FontFamily_getFamilyReleaseFunc }, + { "nAddFont", "(JLjava/nio/ByteBuffer;III)Z", (void*)FontFamily_addFont }, + { "nAddFontWeightStyle", "(JLjava/nio/ByteBuffer;III)Z", (void*)FontFamily_addFontWeightStyle }, { "nAddFontFromAssetManager", "(JLandroid/content/res/AssetManager;Ljava/lang/String;IZIII)Z", (void*)FontFamily_addFontFromAssetManager }, diff --git a/core/jni/android/graphics/Typeface.cpp b/core/jni/android/graphics/Typeface.cpp index d67c0b018efb..34ec365c0bdc 100644 --- a/core/jni/android/graphics/Typeface.cpp +++ b/core/jni/android/graphics/Typeface.cpp @@ -28,8 +28,16 @@ using namespace android; +static inline Typeface* toTypeface(jlong ptr) { + return reinterpret_cast(ptr); +} + +template static inline jlong toJLong(Ptr ptr) { + return reinterpret_cast(ptr); +} + static jlong Typeface_createFromTypeface(JNIEnv* env, jobject, jlong familyHandle, jint style) { - Typeface* family = reinterpret_cast(familyHandle); + Typeface* family = toTypeface(familyHandle); Typeface* face = Typeface::createRelative(family, (Typeface::Style)style); // TODO: the following logic shouldn't be necessary, the above should always succeed. // Try to find the closest matching font, using the standard heuristic @@ -39,13 +47,12 @@ static jlong Typeface_createFromTypeface(JNIEnv* env, jobject, jlong familyHandl for (int i = 0; NULL == face && i < 4; i++) { face = Typeface::createRelative(family, (Typeface::Style)i); } - return reinterpret_cast(face); + return toJLong(face); } static jlong Typeface_createFromTypefaceWithExactStyle(JNIEnv* env, jobject, jlong nativeInstance, jint weight, jboolean italic) { - Typeface* baseTypeface = reinterpret_cast(nativeInstance); - return reinterpret_cast(Typeface::createAbsolute(baseTypeface, weight, italic)); + return toJLong(Typeface::createAbsolute(toTypeface(nativeInstance), weight, italic)); } static jlong Typeface_createFromTypefaceWithVariation(JNIEnv* env, jobject, jlong familyHandle, @@ -60,30 +67,30 @@ static jlong Typeface_createFromTypefaceWithVariation(JNIEnv* env, jobject, jlon AxisHelper axis(env, axisObject); variations.push_back(minikin::FontVariation(axis.getTag(), axis.getStyleValue())); } - Typeface* baseTypeface = reinterpret_cast(familyHandle); - Typeface* result = Typeface::createFromTypefaceWithVariation(baseTypeface, variations); - return reinterpret_cast(result); + return toJLong(Typeface::createFromTypefaceWithVariation(toTypeface(familyHandle), variations)); } static jlong Typeface_createWeightAlias(JNIEnv* env, jobject, jlong familyHandle, jint weight) { - Typeface* family = reinterpret_cast(familyHandle); - Typeface* face = Typeface::createWithDifferentBaseWeight(family, weight); - return reinterpret_cast(face); + return toJLong(Typeface::createWithDifferentBaseWeight(toTypeface(familyHandle), weight)); +} + +static void releaseFunc(jlong ptr) { + delete toTypeface(ptr); } -static void Typeface_unref(JNIEnv* env, jobject obj, jlong faceHandle) { - Typeface* face = reinterpret_cast(faceHandle); - delete face; +// CriticalNative +static jlong Typeface_getReleaseFunc() { + return toJLong(&releaseFunc); } -static jint Typeface_getStyle(JNIEnv* env, jobject obj, jlong faceHandle) { - Typeface* face = reinterpret_cast(faceHandle); - return face->fAPIStyle; +// CriticalNative +static jint Typeface_getStyle(jlong faceHandle) { + return toTypeface(faceHandle)->fAPIStyle; } -static jint Typeface_getWeight(JNIEnv* env, jobject obj, jlong faceHandle) { - Typeface* face = reinterpret_cast(faceHandle); - return face->fStyle.weight(); +// CriticalNative +static jint Typeface_getWeight(jlong faceHandle) { + return toTypeface(faceHandle)->fStyle.weight(); } static jlong Typeface_createFromArray(JNIEnv *env, jobject, jlongArray familyArray, @@ -95,17 +102,16 @@ static jlong Typeface_createFromArray(JNIEnv *env, jobject, jlongArray familyArr FontFamilyWrapper* family = reinterpret_cast(families[i]); familyVec.emplace_back(family->family); } - return reinterpret_cast( - Typeface::createFromFamilies(std::move(familyVec), weight, italic)); + return toJLong(Typeface::createFromFamilies(std::move(familyVec), weight, italic)); } -static void Typeface_setDefault(JNIEnv *env, jobject, jlong faceHandle) { - Typeface* face = reinterpret_cast(faceHandle); - Typeface::setDefault(face); +// CriticalNative +static void Typeface_setDefault(jlong faceHandle) { + Typeface::setDefault(toTypeface(faceHandle)); } static jobject Typeface_getSupportedAxes(JNIEnv *env, jobject, jlong faceHandle) { - Typeface* face = reinterpret_cast(faceHandle); + Typeface* face = toTypeface(faceHandle); const std::unordered_set& tagSet = face->fFontCollection->getSupportedTags(); const size_t length = tagSet.size(); if (length == 0) { @@ -131,7 +137,7 @@ static const JNINativeMethod gTypefaceMethods[] = { { "nativeCreateFromTypefaceWithVariation", "(JLjava/util/List;)J", (void*)Typeface_createFromTypefaceWithVariation }, { "nativeCreateWeightAlias", "(JI)J", (void*)Typeface_createWeightAlias }, - { "nativeUnref", "(J)V", (void*)Typeface_unref }, + { "nativeGetReleaseFunc", "()J", (void*)Typeface_getReleaseFunc }, { "nativeGetStyle", "(J)I", (void*)Typeface_getStyle }, { "nativeGetWeight", "(J)I", (void*)Typeface_getWeight }, { "nativeCreateFromArray", "([JII)J", diff --git a/graphics/java/android/graphics/FontFamily.java b/graphics/java/android/graphics/FontFamily.java index e7cfcfdf7760..c69eb32148c0 100644 --- a/graphics/java/android/graphics/FontFamily.java +++ b/graphics/java/android/graphics/FontFamily.java @@ -24,6 +24,8 @@ import android.util.Log; import dalvik.annotation.optimization.CriticalNative; +import libcore.util.NativeAllocationRegistry; + import java.io.FileInputStream; import java.io.IOException; import java.nio.ByteBuffer; @@ -38,6 +40,14 @@ public class FontFamily { private static String TAG = "FontFamily"; + private static final NativeAllocationRegistry sBuilderRegistry = new NativeAllocationRegistry( + FontFamily.class.getClassLoader(), nGetBuilderReleaseFunc(), 64); + + private @Nullable Runnable mNativeBuilderCleaner; + + private static final NativeAllocationRegistry sFamilyRegistry = new NativeAllocationRegistry( + FontFamily.class.getClassLoader(), nGetFamilyReleaseFunc(), 64); + /** * @hide */ @@ -48,6 +58,7 @@ public class FontFamily { public FontFamily() { mBuilderPtr = nInitBuilder(null, 0); + mNativeBuilderCleaner = sBuilderRegistry.registerNativeAllocation(this, mBuilderPtr); } public FontFamily(@Nullable String[] langs, int variant) { @@ -60,6 +71,7 @@ public class FontFamily { langsString = TextUtils.join(",", langs); } mBuilderPtr = nInitBuilder(langsString, variant); + mNativeBuilderCleaner = sBuilderRegistry.registerNativeAllocation(this, mBuilderPtr); } /** @@ -73,7 +85,11 @@ public class FontFamily { throw new IllegalStateException("This FontFamily is already frozen"); } mNativePtr = nCreateFamily(mBuilderPtr); + mNativeBuilderCleaner.run(); mBuilderPtr = 0; + if (mNativePtr != 0) { + sFamilyRegistry.registerNativeAllocation(this, mNativePtr); + } return mNativePtr != 0; } @@ -81,24 +97,10 @@ public class FontFamily { if (mBuilderPtr == 0) { throw new IllegalStateException("This FontFamily is already frozen or abandoned"); } - nAbort(mBuilderPtr); + mNativeBuilderCleaner.run(); mBuilderPtr = 0; } - @Override - protected void finalize() throws Throwable { - try { - if (mNativePtr != 0) { - nUnrefFamily(mNativePtr); - } - if (mBuilderPtr != 0) { - nAbort(mBuilderPtr); - } - } finally { - super.finalize(); - } - } - public boolean addFont(String path, int ttcIndex, FontVariationAxis[] axes, int weight, int italic) { if (mBuilderPtr == 0) { @@ -171,10 +173,10 @@ public class FontFamily { private static native long nCreateFamily(long mBuilderPtr); @CriticalNative - private static native void nAbort(long mBuilderPtr); + private static native long nGetBuilderReleaseFunc(); @CriticalNative - private static native void nUnrefFamily(long nativePtr); + private static native long nGetFamilyReleaseFunc(); // By passing -1 to weigth argument, the weight value is resolved by OS/2 table in the font. // By passing -1 to italic argument, the italic value is resolved by OS/2 table in the font. private static native boolean nAddFont(long builderPtr, ByteBuffer font, int ttcIndex, diff --git a/graphics/java/android/graphics/Typeface.java b/graphics/java/android/graphics/Typeface.java index 38beebde4b14..f27c11dbf735 100644 --- a/graphics/java/android/graphics/Typeface.java +++ b/graphics/java/android/graphics/Typeface.java @@ -42,6 +42,10 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.Preconditions; +import dalvik.annotation.optimization.CriticalNative; + +import libcore.util.NativeAllocationRegistry; + import org.xmlpull.v1.XmlPullParserException; import java.io.File; @@ -71,6 +75,9 @@ public class Typeface { private static String TAG = "Typeface"; + private static final NativeAllocationRegistry sRegistry = new NativeAllocationRegistry( + Typeface.class.getClassLoader(), nativeGetReleaseFunc(), 64); + /** The default NORMAL typeface object */ public static final Typeface DEFAULT; /** @@ -911,6 +918,7 @@ public class Typeface { } native_instance = ni; + sRegistry.registerNativeAllocation(this, native_instance); mStyle = nativeGetStyle(ni); mWeight = nativeGetWeight(ni); } @@ -1120,16 +1128,6 @@ public class Typeface { } @Override - protected void finalize() throws Throwable { - try { - nativeUnref(native_instance); - native_instance = 0; // Other finalizers can still call us. - } finally { - super.finalize(); - } - } - - @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; @@ -1173,10 +1171,18 @@ public class Typeface { private static native long nativeCreateFromTypefaceWithVariation( long native_instance, List axes); private static native long nativeCreateWeightAlias(long native_instance, int weight); - private static native void nativeUnref(long native_instance); - private static native int nativeGetStyle(long native_instance); - private static native int nativeGetWeight(long native_instance); private static native long nativeCreateFromArray(long[] familyArray, int weight, int italic); - private static native void nativeSetDefault(long native_instance); private static native int[] nativeGetSupportedAxes(long native_instance); + + @CriticalNative + private static native void nativeSetDefault(long nativePtr); + + @CriticalNative + private static native int nativeGetStyle(long nativePtr); + + @CriticalNative + private static native int nativeGetWeight(long nativePtr); + + @CriticalNative + private static native long nativeGetReleaseFunc(); } -- 2.11.0