From 31bf860bb645516c20e8920ca2e5e2fac6353bd8 Mon Sep 17 00:00:00 2001
From: Seigo Nonaka <nona@google.com>
Date: Wed, 14 Oct 2020 15:04:01 -0700
Subject: [PATCH] Improve Font#equals and Font#hashCode for HashMap.

Font#equals and Font#hashCode reads bytes in the buffer and end up
with bad performance if we use it as key of HashMap.

To improve them, does following.

- Font#hashCode
Use buffer size as hash. Since we only accept OpenType compliant font
files (invalid font data will be IllegalArgumentException in ctor),
the hash collision due to same file size is unlikely happens.

The ByteBuffer#hashCode includes head position as hashCode but
intentionally exclude them from new hashCode since the head position
is not used by platform include native code.

- Font#equals
The ByteBuffer is frequently duplicated for many reasons, e.g. having
different reading head, etc, but the underlying mmaped memory address
has not changed. Fortunately we have this address in underlying font
object. Compare them as a shortcut of content equality.

With this change, the local benchmark for the Font key-ed HashMap look
up gets faster from 30ms to 4us. (on Pixel 3a)

Bug: 170494256
Test: atest FontTest

Change-Id: Ibc7f7aae0b3427e71209444eebb93753c340b711
---
 .../java/android/graphics/fonts/Font.java     | 47 +++++++++++++++++--
 libs/hwui/jni/fonts/Font.cpp                  | 11 +++++
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/graphics/java/android/graphics/fonts/Font.java b/graphics/java/android/graphics/fonts/Font.java
index 97cd8ab6cae93..586c512b3f97e 100644
--- a/graphics/java/android/graphics/fonts/Font.java
+++ b/graphics/java/android/graphics/fonts/Font.java
@@ -523,6 +523,9 @@ public final class Font {
     /**
      * Returns a font file buffer.
      *
+     * Duplicate before reading values by {@link ByteBuffer#duplicate()} for avoiding unexpected
+     * reading position sharing.
+     *
      * @return a font buffer
      */
     public @NonNull ByteBuffer getBuffer() {
@@ -628,18 +631,49 @@ public final class Font {
         if (o == this) {
             return true;
         }
-        if (o == null || !(o instanceof Font)) {
+        if (!(o instanceof Font)) {
             return false;
         }
         Font f = (Font) o;
-        return mFontStyle.equals(f.mFontStyle) && f.mTtcIndex == mTtcIndex
-                && Arrays.equals(f.mAxes, mAxes) && f.mBuffer.equals(mBuffer)
-                && Objects.equals(f.mLocaleList, mLocaleList);
+        boolean paramEqual = mFontStyle.equals(f.mFontStyle) && f.mTtcIndex == mTtcIndex
+                && Arrays.equals(f.mAxes, mAxes) && Objects.equals(f.mLocaleList, mLocaleList)
+                && Objects.equals(mFile, f.mFile);
+
+        if (!paramEqual) {
+            return false;
+        }
+
+        // Shortcut for different font buffer check by comparing size.
+        if (mBuffer.capacity() != f.mBuffer.capacity()) {
+            return false;
+        }
+
+        // ByteBuffer#equals compares all bytes which is not performant for e.g HashMap. Since
+        // underlying native font object holds buffer address, check if this buffer points exactly
+        // the same address as a shortcut of equality. For being compatible with of API30 or before,
+        // check buffer position even if the buffer points the same address.
+        if (nIsSameBufferAddress(mNativePtr, f.mNativePtr)
+                && mBuffer.position() == f.mBuffer.position()) {
+            return true;
+        }
+
+        // Unfortunately, need to compare bytes one-by-one since the buffer may be different font
+        // file but has the same file size, or two font has same content but they are allocated
+        // differently. For being compatible with API30 ore before, compare with ByteBuffer#equals.
+        return mBuffer.equals(f.mBuffer);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(mFontStyle, mTtcIndex, Arrays.hashCode(mAxes), mBuffer, mLocaleList);
+        return Objects.hash(
+                mFontStyle,
+                mTtcIndex,
+                Arrays.hashCode(mAxes),
+                // Use Buffer size instead of ByteBuffer#hashCode since ByteBuffer#hashCode traverse
+                // data which is not performant e.g. for HashMap. The hash collision are less likely
+                // happens because it is unlikely happens the different font files has exactly the
+                // same size.
+                mLocaleList);
     }
 
     @Override
@@ -724,4 +758,7 @@ public final class Font {
 
     @CriticalNative
     private static native long nGetNativeFontPtr(long ptr);
+
+    @CriticalNative
+    private static native boolean nIsSameBufferAddress(long lFontPtr, long rFontPtr);
 }
diff --git a/libs/hwui/jni/fonts/Font.cpp b/libs/hwui/jni/fonts/Font.cpp
index aeb096df141a2..4aee6b94a2beb 100644
--- a/libs/hwui/jni/fonts/Font.cpp
+++ b/libs/hwui/jni/fonts/Font.cpp
@@ -238,6 +238,16 @@ static jlong Font_getNativeFontPtr(CRITICAL_JNI_PARAMS_COMMA jlong fontHandle) {
     return reinterpret_cast<jlong>(font->font.get());
 }
 
+// Critical Native
+static jboolean Font_isSameBufferAddress(CRITICAL_JNI_PARAMS_COMMA jlong lFontHandle,
+                                         jlong rFontHandle) {
+    FontWrapper* lFont = reinterpret_cast<FontWrapper*>(lFontHandle);
+    FontWrapper* rFont = reinterpret_cast<FontWrapper*>(rFontHandle);
+    const void* lBufferPtr = lFont->font->typeface()->GetFontData();
+    const void* rBufferPtr = rFont->font->typeface()->GetFontData();
+    return lBufferPtr == rBufferPtr;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 struct FontBufferWrapper {
@@ -287,6 +297,7 @@ static const JNINativeMethod gFontMethods[] = {
     { "nGetAxisInfo", "(JI)J", (void*) Font_getAxisInfo },
     { "nGetFontPath", "(J)Ljava/lang/String;", (void*) Font_getFontPath },
     { "nGetNativeFontPtr", "(J)J", (void*) Font_getNativeFontPtr },
+    { "nIsSameBufferAddress", "(JJ)Z", (void*) Font_isSameBufferAddress },
 };
 
 static const JNINativeMethod gFontBufferHelperMethods[] = {