From cdab0a89104302a5b9914056053313794e3947b1 Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Mon, 18 Jun 2012 11:09:43 -0700 Subject: [PATCH] Bug 762978 - Make AndroidBrowserRepositorySession.recordToGuid map from hashes of record strings to record guids to reduce memory pressure. r=liuche --- .../AndroidBrowserRepositorySession.java | 94 +++++++++++++++++-- 1 file changed, 84 insertions(+), 10 deletions(-) diff --git a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java index e7773569709..54d8eb71595 100644 --- a/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java +++ b/mobile/android/base/sync/repositories/android/AndroidBrowserRepositorySession.java @@ -57,7 +57,21 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos public static final String LOG_TAG = "BrowserRepoSession"; protected AndroidBrowserRepositoryDataAccessor dbHelper; - private HashMap recordToGuid; + + /** + * In order to reconcile the "same record" with two *different* GUIDs (for + * example, the same bookmark created by two different clients), we maintain a + * mapping for each local record from a "record string" to + * "local record GUID". + *

+ * The "record string" above is a "record identifying unique key" produced by + * buildRecordString. + *

+ * Since we hash each "record string", this map may produce a false positive. + * In this case, we search the database for a matching record explicitly using + * findByRecordString. + */ + protected HashMap recordToGuid; public AndroidBrowserRepositorySession(Repository repository) { super(repository); @@ -156,6 +170,13 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos super.finish(delegate); } + /** + * Produce a "record string" (record identifying unique key). + * + * @param record + * the Record to identify. + * @return a String instance. + */ protected abstract String buildRecordString(Record record); protected void checkDatabase() throws ProfileDatabaseException, NullCursorException { @@ -626,26 +647,40 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos } Logger.debug(LOG_TAG, "Searching with record string " + recordString); - String guid = getRecordToGuidMap().get(recordString); + String guid = getGuidForString(recordString); if (guid == null) { Logger.debug(LOG_TAG, "findExistingRecord failed to find one for " + record.guid); return null; } - Logger.debug(LOG_TAG, "Found one. Returning computed record."); - return retrieveByGUIDDuringStore(guid); + // Our map contained a match, but it could be a false positive. Since + // computed record string is supposed to be a unique key, we can easily + // verify our positive. + Logger.debug(LOG_TAG, "Found one. Checking stored record."); + Record stored = retrieveByGUIDDuringStore(guid); + String storedRecordString = buildRecordString(record); + if (recordString.equals(storedRecordString)) { + Logger.debug(LOG_TAG, "Existing record matches incoming record. Returning existing record."); + return stored; + } + + // Oh no, we got a false positive! (This should be *very* rare -- + // essentially, we got a hash collision.) Search the DB for this record + // explicitly by hand. + Logger.debug(LOG_TAG, "Existing record does not match incoming record. Trying to find record by record string."); + return findByRecordString(recordString); } - public HashMap getRecordToGuidMap() throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + protected String getGuidForString(String recordString) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { if (recordToGuid == null) { createRecordToGuidMap(); } - return recordToGuid; + return recordToGuid.get(new Integer(recordString.hashCode())); } - private void createRecordToGuidMap() throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + protected void createRecordToGuidMap() throws NoGuidForIdException, NullCursorException, ParentNotFoundException { Logger.info(LOG_TAG, "BEGIN: creating record -> GUID map."); - recordToGuid = new HashMap(); + recordToGuid = new HashMap(); // TODO: we should be able to do this entire thing with string concatenations within SQL. // Also consider whether it's better to fetch and process every record in the DB into @@ -660,7 +695,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos if (record != null) { final String recordString = buildRecordString(record); if (recordString != null) { - recordToGuid.put(recordString, record.guid); + recordToGuid.put(new Integer(recordString.hashCode()), record.guid); } } cur.moveToNext(); @@ -671,6 +706,45 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos Logger.info(LOG_TAG, "END: creating record -> GUID map."); } + /** + * Search the local database for a record with the same "record string". + *

+ * We expect to do this only in the unlikely event of a hash + * collision, so we iterate the database completely. Since we want + * to include information about the parents of bookmarks, it is + * difficult to do better purely using the + * ContentProvider interface. + * + * @param recordString + * the "record string" to search for; must be n + * @return a Record with the same "record string", or + * null if none is present. + * @throws ParentNotFoundException + * @throws NullCursorException + * @throws NoGuidForIdException + */ + protected Record findByRecordString(String recordString) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { + Cursor cur = dbHelper.fetchAll(); + try { + if (!cur.moveToFirst()) { + return null; + } + while (!cur.isAfterLast()) { + Record record = retrieveDuringStore(cur); + if (record != null) { + final String storedRecordString = buildRecordString(record); + if (recordString.equals(storedRecordString)) { + return record; + } + } + cur.moveToNext(); + } + return null; + } finally { + cur.close(); + } + } + public void putRecordToGuidMap(String recordString, String guid) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { if (recordString == null) { return; @@ -679,7 +753,7 @@ public abstract class AndroidBrowserRepositorySession extends StoreTrackingRepos if (recordToGuid == null) { createRecordToGuidMap(); } - recordToGuid.put(recordString, guid); + recordToGuid.put(new Integer(recordString.hashCode()), guid); } protected abstract Record prepareRecord(Record record);