Remove indentation for Person.toString
Instead of using GenericDocument.toString, we provide a customized
String builder to build the string without indentation for fingerprinting.
The builder is added in PersonBuilderHelper, and duplicates some codes
from GenericDocument#appendGenericDocumentString and
GenericDocument#appendPropertyString, with indenting and some formatting
removed.
In GenericDocument.toString, in order to do the indentation, the first
'\n' is searched over, and we will split the string into two part, and
recursively do the same thing on the substring after '\n'. This is causing stack overflow and big memory consumption if we are getting a big string with a lot of '\n'. It seems there is no optimization for tail recursion here, and calling String.substring will create a copy of sub-string and increase the memory usage.
Memory Test(calling Person.toString many times with big notes before and
after the fix aosp/2413038):
Before: memory usage increase a lot after tests start:
before test started: https://screenshot.googleplex.com/9hrUZrwRzMA28ym
after test started: https://screenshot.googleplex.com/9jKWjy7QKtp2Kqp
After: stable memory usage throughout the test.
https://screenshot.googleplex.com/AyxFnr9TPqGbjUX
After a device gets this change, the index won't be affected, except
that one unnecessary update may occur for each indexed contact, because
the fingerprint will be changed even if the content is the same. This
additional update per contact may happen during either delta update(some
fields we don't care get updated in CP2, thus we get notified), or
during a full update. In full update case, all the existing contacts
will be updated.
This approach is preferred compared with completely re-implementing
toString because we don't want to duplicate the methods in
GenericDocument, which will make it harder to maintain.
Bug: b/263885339
Test: ContactsIndexerTests
Change-Id: Ib3f10630344579cb9ffbb2bfe4685beaae8cb709
(cherry picked from commit 2a3a840126e85d197a0f03c5f86ae3fdd0c68522)
Merged-In: Ib3f10630344579cb9ffbb2bfe4685beaae8cb709
diff --git a/service/java/com/android/server/appsearch/contactsindexer/PersonBuilderHelper.java b/service/java/com/android/server/appsearch/contactsindexer/PersonBuilderHelper.java
index 027fea3..be9ac4a 100644
--- a/service/java/com/android/server/appsearch/contactsindexer/PersonBuilderHelper.java
+++ b/service/java/com/android/server/appsearch/contactsindexer/PersonBuilderHelper.java
@@ -17,23 +17,27 @@
package com.android.server.appsearch.contactsindexer;
import android.annotation.NonNull;
+import android.app.appsearch.GenericDocument;
+import android.app.appsearch.util.IndentingStringBuilder;
import android.app.appsearch.util.LogUtil;
import android.util.ArrayMap;
import android.util.Log;
+import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.util.Preconditions;
import com.android.server.appsearch.contactsindexer.appsearchtypes.ContactPoint;
import com.android.server.appsearch.contactsindexer.appsearchtypes.Person;
+import java.lang.reflect.Array;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Objects;
-import com.android.internal.util.Preconditions;
-
/**
* Helper class to help build the {@link Person}.
*
@@ -211,7 +215,99 @@
Objects.requireNonNull(person);
MessageDigest md = MessageDigest.getInstance("MD5");
- md.update(person.toString().getBytes(StandardCharsets.UTF_8));
+ md.update(generateFingerprintStringForPerson(person).getBytes(StandardCharsets.UTF_8));
return md.digest();
}
+
+ @VisibleForTesting
+ /** Returns a string presentation of {@link Person} for fingerprinting. */
+ static String generateFingerprintStringForPerson(@NonNull Person person) {
+ Objects.requireNonNull(person);
+
+ StringBuilder builder = new StringBuilder();
+ appendGenericDocumentString(person, builder);
+ return builder.toString();
+ }
+
+ /**
+ * Appends string representation of a {@link GenericDocument} to the {@link StringBuilder}.
+ *
+ * <p>This is basically same as
+ * {@link GenericDocument#appendGenericDocumentString(IndentingStringBuilder)}, but only keep
+ * the properties part and use a normal {@link StringBuilder} to skip the indentation.
+ */
+ private static void appendGenericDocumentString(@NonNull GenericDocument doc,
+ @NonNull StringBuilder builder) {
+ Objects.requireNonNull(doc);
+ Objects.requireNonNull(builder);
+
+ builder.append("properties: {\n");
+ String[] sortedProperties = doc.getPropertyNames().toArray(new String[0]);
+ Arrays.sort(sortedProperties);
+ for (int i = 0; i < sortedProperties.length; i++) {
+ Object property = Objects.requireNonNull(doc.getProperty(sortedProperties[i]));
+ appendPropertyString(sortedProperties[i], property, builder);
+ if (i != sortedProperties.length - 1) {
+ builder.append(",\n");
+ }
+ }
+ builder.append("\n");
+ builder.append("}");
+ }
+
+ /**
+ * Appends string representation of a {@link GenericDocument}'s property to the
+ * {@link StringBuilder}.
+ *
+ * <p>This is basically same as
+ * {@link GenericDocument#appendPropertyString(String, Object, IndentingStringBuilder)}, but
+ * use a normal {@link StringBuilder} to skip the indentation.
+ *
+ * <p>Here we still keep most of the formatting(e.g. '\n') to make sure we won't hit some
+ * possible corner cases. E.g. We will have "someProperty1: some\n Property2:..." instead of
+ * "someProperty1: someProperty2:". For latter, we can interpret it as empty string value for
+ * "someProperty1", with a different property name "someProperty2". In this case, the content is
+ * changed but fingerprint will remain same if we don't have that '\n'.
+ *
+ * <p>Plus, some basic formatting will make the testing more clear.
+ */
+ private static void appendPropertyString(
+ @NonNull String propertyName,
+ @NonNull Object property,
+ @NonNull StringBuilder builder) {
+ Objects.requireNonNull(propertyName);
+ Objects.requireNonNull(property);
+ Objects.requireNonNull(builder);
+
+ builder.append("\"").append(propertyName).append("\": [");
+ if (property instanceof GenericDocument[]) {
+ GenericDocument[] documentValues = (GenericDocument[]) property;
+ for (int i = 0; i < documentValues.length; ++i) {
+ builder.append("\n");
+ appendGenericDocumentString(documentValues[i], builder);
+ if (i != documentValues.length - 1) {
+ builder.append(",");
+ }
+ builder.append("\n");
+ }
+ builder.append("]");
+ } else {
+ int propertyArrLength = Array.getLength(property);
+ for (int i = 0; i < propertyArrLength; i++) {
+ Object propertyElement = Array.get(property, i);
+ if (propertyElement instanceof String) {
+ builder.append("\"").append((String) propertyElement).append("\"");
+ } else if (propertyElement instanceof byte[]) {
+ builder.append(Arrays.toString((byte[]) propertyElement));
+ } else {
+ builder.append(propertyElement.toString());
+ }
+ if (i != propertyArrLength - 1) {
+ builder.append(", ");
+ } else {
+ builder.append("]");
+ }
+ }
+ }
+ }
}
diff --git a/testing/contactsindexertests/Android.bp b/testing/contactsindexertests/Android.bp
index 7683dac..4250e08 100644
--- a/testing/contactsindexertests/Android.bp
+++ b/testing/contactsindexertests/Android.bp
@@ -30,9 +30,9 @@
],
libs: [
"android.test.runner",
- "framework-appsearch.impl",
"android.test.mock",
"android.test.base",
+ "framework-appsearch.impl",
],
test_suites: [
"general-tests",
diff --git a/testing/contactsindexertests/src/com/android/server/appsearch/contactsindexer/PersonBuilderHelperTest.java b/testing/contactsindexertests/src/com/android/server/appsearch/contactsindexer/PersonBuilderHelperTest.java
index b52aacf..e2f63c3 100644
--- a/testing/contactsindexertests/src/com/android/server/appsearch/contactsindexer/PersonBuilderHelperTest.java
+++ b/testing/contactsindexertests/src/com/android/server/appsearch/contactsindexer/PersonBuilderHelperTest.java
@@ -16,14 +16,14 @@
package com.android.server.appsearch.contactsindexer;
+import static com.google.common.truth.Truth.assertThat;
+
import android.net.Uri;
-import com.android.server.appsearch.contactsindexer.PersonBuilderHelper;
import com.android.server.appsearch.contactsindexer.appsearchtypes.ContactPoint;
import com.android.server.appsearch.contactsindexer.appsearchtypes.Person;
-
-import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableList;
import org.junit.Test;
@@ -278,4 +278,122 @@
// Score should be set as base(1) + # of contactPoints + # of additionalNames.
assertThat(person.getScore()).isEqualTo(6);
}
+
+ @Test
+ public void testGenerateFingerprintStringForPerson() {
+ long creationTimestamp = 12345L;
+ String namespace = "namespace";
+ String id = "id";
+ int score = 3;
+ String name = "name";
+ String givenName = "givenName";
+ String middleName = "middleName";
+ String lastName = "lastName";
+ Uri externalUri = Uri.parse("http://external.com");
+ Uri imageUri = Uri.parse("http://image.com");
+ byte[] fingerprint = "Hello world!".getBytes();
+ List<String> affiliations = ImmutableList.of("Org1", "Org2", "Org3");
+ List<String> relations = ImmutableList.of("relation1", "relation2");
+ boolean isImportant = true;
+ boolean isBot = true;
+ String note1 = "note";
+ String note2 = "note2";
+ ContactPoint contact1 = new ContactPoint.Builder(namespace, id + "1", "Home")
+ .setCreationTimestampMillis(creationTimestamp)
+ .addAddress("addr1")
+ .addPhone("phone1")
+ .addEmail("email1")
+ .addAppId("appId1")
+ .build();
+ ContactPoint contact2 = new ContactPoint.Builder(namespace, id + "2", "Work")
+ .setCreationTimestampMillis(creationTimestamp)
+ .addAddress("addr2")
+ .addPhone("phone2")
+ .addEmail("email2")
+ .addAppId("appId2")
+ .build();
+ ContactPoint contact3 = new ContactPoint.Builder(namespace, id + "3", "Other")
+ .setCreationTimestampMillis(creationTimestamp)
+ .addAddress("addr3")
+ .addPhone("phone3")
+ .addEmail("email3")
+ .addAppId("appId3")
+ .build();
+ List<String> additionalNames = ImmutableList.of("nickname", "phoneticName");
+ @Person.NameType
+ List<Long> additionalNameTypes = ImmutableList.of((long) Person.TYPE_NICKNAME,
+ (long) Person.TYPE_PHONETIC_NAME);
+ Person person = new Person.Builder(namespace, id, name)
+ .setCreationTimestampMillis(creationTimestamp)
+ .setScore(score)
+ .setGivenName(givenName)
+ .setMiddleName(middleName)
+ .setFamilyName(lastName)
+ .setExternalUri(externalUri)
+ .setImageUri(imageUri)
+ .addAdditionalName(additionalNameTypes.get(0), additionalNames.get(0))
+ .addAdditionalName(additionalNameTypes.get(1), additionalNames.get(1))
+ .addAffiliation(affiliations.get(0))
+ .addAffiliation(affiliations.get(1))
+ .addAffiliation(affiliations.get(2))
+ .addRelation(relations.get(0))
+ .addRelation(relations.get(1))
+ .setIsImportant(isImportant)
+ .setIsBot(isBot)
+ .addNote(note1)
+ .addNote(note2)
+ .setFingerprint(fingerprint)
+ .addContactPoint(contact1)
+ .addContactPoint(contact2)
+ .addContactPoint(contact3)
+ .build();
+
+ // Different from GenericDocument.toString, we will a get string representation without
+ // any indentation for Person.
+ String expected = "properties: {\n"
+ + "\"additionalNameTypes\": [1, 2],\n"
+ + "\"additionalNames\": [\"nickname\", \"phoneticName\"],\n"
+ + "\"affiliations\": [\"Org1\", \"Org2\", \"Org3\"],\n"
+ + "\"contactPoints\": [\n"
+ + "properties: {\n"
+ + "\"address\": [\"addr1\"],\n"
+ + "\"appId\": [\"appId1\"],\n"
+ + "\"email\": [\"email1\"],\n"
+ + "\"label\": [\"Home\"],\n"
+ + "\"telephone\": [\"phone1\"]\n"
+ + "},\n"
+ + "\n"
+ + "properties: {\n"
+ + "\"address\": [\"addr2\"],\n"
+ + "\"appId\": [\"appId2\"],\n"
+ + "\"email\": [\"email2\"],\n"
+ + "\"label\": [\"Work\"],\n"
+ + "\"telephone\": [\"phone2\"]\n"
+ + "},\n"
+ + "\n"
+ + "properties: {\n"
+ + "\"address\": [\"addr3\"],\n"
+ + "\"appId\": [\"appId3\"],\n"
+ + "\"email\": [\"email3\"],\n"
+ + "\"label\": [\"Other\"],\n"
+ + "\"telephone\": [\"phone3\"]\n"
+ + "}\n"
+ + "],\n"
+ + "\"externalUri\": [\"http://external.com\"],\n"
+ + "\"familyName\": [\"lastName\"],\n"
+ + "\"fingerprint\": [[72, 101, 108, 108, 111, 32, 119, 111, 114, 108, "
+ + "100, 33]],\n"
+ + "\"givenName\": [\"givenName\"],\n"
+ + "\"imageUri\": [\"http://image.com\"],\n"
+ + "\"isBot\": [true],\n"
+ + "\"isImportant\": [true],\n"
+ + "\"middleName\": [\"middleName\"],\n"
+ + "\"name\": [\"name\"],\n"
+ + "\"notes\": [\"note\", \"note2\"],\n"
+ + "\"relations\": [\"relation1\", \"relation2\"]\n"
+ + "}";
+
+ assertThat(PersonBuilderHelper.generateFingerprintStringForPerson(person)).isEqualTo(
+ expected);
+ }
}