From 4a5a21d6111f611d7ca0ca276f1147a0f643c0b5 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 14:34:00 -0700 Subject: [PATCH 1/3] LocalStore uses a SparseArray to track target ids, not a set --- .../google/firebase/firestore/local/LruDelegate.java | 4 +++- .../firestore/local/LruGarbageCollector.java | 4 +++- .../firestore/local/MemoryLruReferenceDelegate.java | 4 +++- .../firebase/firestore/local/MemoryQueryCache.java | 7 ++++--- .../firestore/local/SQLiteLruReferenceDelegate.java | 4 +++- .../firebase/firestore/local/SQLiteQueryCache.java | 6 ++++-- .../firestore/local/LruGarbageCollectorTestCase.java | 12 +++++++----- 7 files changed, 27 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java index b42d984ce38..50c4718c89e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import com.google.firebase.firestore.util.Consumer; import java.util.Set; @@ -37,7 +39,7 @@ interface LruDelegate { * * @return the number of targets removed. */ - int removeTargets(long upperBound, Set activeTargetIds); + int removeTargets(long upperBound, SparseArray activeTargetIds); /** * Removes all unreferenced documents from the cache that have a sequence number less than or diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java index 18c675d38c3..7c870e0334d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import com.google.firebase.firestore.core.ListenSequence; import java.util.Comparator; import java.util.PriorityQueue; @@ -80,7 +82,7 @@ long nthSequenceNumber(int count) { * Removes targets with a sequence number equal to or less than the given upper bound, and removes * document associations with those targets. */ - int removeTargets(long upperBound, Set activeTargetIds) { + int removeTargets(long upperBound, SparseArray activeTargetIds) { return delegate.removeTargets(upperBound, activeTargetIds); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java index 09bec4032c3..0671065dbcd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.core.ListenSequence; @@ -94,7 +96,7 @@ public void setInMemoryPins(ReferenceSet inMemoryPins) { } @Override - public int removeTargets(long upperBound, Set activeTargetIds) { + public int removeTargets(long upperBound, SparseArray activeTargetIds) { return persistence.getQueryCache().removeQueries(upperBound, activeTargetIds); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java index 3d428e382d3..403344991d0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; @@ -22,7 +24,6 @@ import java.util.HashMap; import java.util.Iterator; import java.util.Map; -import java.util.Set; import javax.annotation.Nullable; /** @@ -115,13 +116,13 @@ public void removeQueryData(QueryData queryData) { * * @return the number of targets removed */ - int removeQueries(long upperBound, Set activeTargetIds) { + int removeQueries(long upperBound, SparseArray activeTargetIds) { int removed = 0; for (Iterator> it = queries.entrySet().iterator(); it.hasNext(); ) { Map.Entry entry = it.next(); int targetId = entry.getValue().getTargetId(); long sequenceNumber = entry.getValue().getSequenceNumber(); - if (sequenceNumber <= upperBound && !activeTargetIds.contains(targetId)) { + if (sequenceNumber <= upperBound && activeTargetIds.get(targetId) == null) { it.remove(); removeMatchingKeysForTargetId(targetId); removed++; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java index f27c929588b..d0bbf98e76d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.core.ListenSequence; @@ -103,7 +105,7 @@ public void removeReference(DocumentKey key) { } @Override - public int removeTargets(long upperBound, Set activeTargetIds) { + public int removeTargets(long upperBound, SparseArray activeTargetIds) { return persistence.getQueryCache().removeQueries(upperBound, activeTargetIds); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java index e3f43da0bc4..496c95eacb2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java @@ -18,6 +18,8 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import android.database.sqlite.SQLiteStatement; +import android.util.SparseArray; + import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; @@ -186,7 +188,7 @@ public void removeQueryData(QueryData queryData) { * present in `activeTargetIds`. Document associations for the removed targets are also removed. * Returns the number of targets removed. */ - int removeQueries(long upperBound, Set activeTargetIds) { + int removeQueries(long upperBound, SparseArray activeTargetIds) { int[] count = new int[1]; // SQLite has a max sql statement size, so there is technically a possibility that including a // an IN clause in this query to filter `activeTargetIds` could overflow. Rather than deal with @@ -196,7 +198,7 @@ int removeQueries(long upperBound, Set activeTargetIds) { .forEach( row -> { int targetId = row.getInt(0); - if (!activeTargetIds.contains(targetId)) { + if (activeTargetIds.get(targetId) == null) { removeTarget(targetId); count[0]++; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java index 5247a1e5a4b..19f51c94db7 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import android.util.SparseArray; + import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.keySet; import static com.google.firebase.firestore.testutil.TestUtil.query; @@ -162,7 +164,7 @@ private void removeDocumentFromTarget(DocumentKey key, int targetId) { queryCache.removeMatchingKeys(keySet(key), targetId); } - private int removeTargets(long upperBound, Set activeTargetIds) { + private int removeTargets(long upperBound, SparseArray activeTargetIds) { return persistence.runTransaction( "Remove queries", () -> garbageCollector.removeTargets(upperBound, activeTargetIds)); } @@ -283,7 +285,7 @@ public void testSequenceNumbersWithMutationsInQueries() { @Test public void testRemoveQueriesUpThroughSequenceNumber() { - Map activeTargetIds = new HashMap<>(); + SparseArray activeTargetIds = new SparseArray<>(); for (int i = 0; i < 100; i++) { QueryData queryData = addNextQuery(); // Mark odd queries as live so we can test filtering out live queries. @@ -295,7 +297,7 @@ public void testRemoveQueriesUpThroughSequenceNumber() { // GC up through 20th query, which is 20%. // Expect to have GC'd 10 targets, since every other target is live long upperBound = 20 + initialSequenceNumber; - int removed = removeTargets(upperBound, activeTargetIds.keySet()); + int removed = removeTargets(upperBound, activeTargetIds); assertEquals(10, removed); // Make sure we removed the even targets with targetID <= 20. persistence.runTransaction( @@ -567,8 +569,8 @@ public void testRemoveTargetsThenGC() { }); // Finally, do the garbage collection, up to but not including the removal of middleTarget - Set activeTargetIds = new HashSet<>(); - activeTargetIds.add(oldestTarget.getTargetId()); + SparseArray activeTargetIds = new SparseArray<>(); + activeTargetIds.put(oldestTarget.getTargetId(), oldestTarget); int targetsRemoved = garbageCollector.removeTargets(upperBound, activeTargetIds); // Expect to remove newest target assertEquals(1, targetsRemoved); From 92cd529154dd5529ac261c13c960d7ea14e3d180 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 14:51:37 -0700 Subject: [PATCH 2/3] Add android stuff to memory version of the lru tests --- .../firestore/local/MemoryLruGarbageCollectorTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLruGarbageCollectorTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLruGarbageCollectorTest.java index 2b9490dcb1e..98074286836 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLruGarbageCollectorTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLruGarbageCollectorTest.java @@ -14,6 +14,12 @@ package com.google.firebase.firestore.local; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) public class MemoryLruGarbageCollectorTest extends LruGarbageCollectorTestCase { @Override Persistence createPersistence() { From 0112fa723eb1f79f1db493b5c14bfe90a59c8335 Mon Sep 17 00:00:00 2001 From: Greg Soltis Date: Mon, 8 Oct 2018 15:05:17 -0700 Subject: [PATCH 3/3] Formatting --- .../java/com/google/firebase/firestore/local/LruDelegate.java | 2 -- .../google/firebase/firestore/local/LruGarbageCollector.java | 2 -- .../firebase/firestore/local/MemoryLruReferenceDelegate.java | 4 +--- .../com/google/firebase/firestore/local/MemoryQueryCache.java | 1 - .../firebase/firestore/local/SQLiteLruReferenceDelegate.java | 4 +--- .../com/google/firebase/firestore/local/SQLiteQueryCache.java | 2 -- .../firebase/firestore/local/LruGarbageCollectorTestCase.java | 3 +-- 7 files changed, 3 insertions(+), 15 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java index 50c4718c89e..569ef9415ea 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java @@ -15,9 +15,7 @@ package com.google.firebase.firestore.local; import android.util.SparseArray; - import com.google.firebase.firestore.util.Consumer; -import java.util.Set; /** * Persistence layers intending to use LRU Garbage collection should implement this interface. This diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java index 7c870e0334d..91a61731338 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java @@ -15,11 +15,9 @@ package com.google.firebase.firestore.local; import android.util.SparseArray; - import com.google.firebase.firestore.core.ListenSequence; import java.util.Comparator; import java.util.PriorityQueue; -import java.util.Set; /** Implements the steps for LRU garbage collection. */ class LruGarbageCollector { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java index 0671065dbcd..5376b5f9378 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java @@ -14,17 +14,15 @@ package com.google.firebase.firestore.local; -import android.util.SparseArray; - import static com.google.firebase.firestore.util.Assert.hardAssert; +import android.util.SparseArray; import com.google.firebase.firestore.core.ListenSequence; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.util.Consumer; import java.util.HashMap; import java.util.Map; -import java.util.Set; /** Provides LRU garbage collection functionality for MemoryPersistence. */ class MemoryLruReferenceDelegate implements ReferenceDelegate, LruDelegate { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java index 403344991d0..e37a451810d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java @@ -15,7 +15,6 @@ package com.google.firebase.firestore.local; import android.util.SparseArray; - import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java index d0bbf98e76d..cee90c741cc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java @@ -14,15 +14,13 @@ package com.google.firebase.firestore.local; -import android.util.SparseArray; - import static com.google.firebase.firestore.util.Assert.hardAssert; +import android.util.SparseArray; import com.google.firebase.firestore.core.ListenSequence; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.util.Consumer; -import java.util.Set; /** Provides LRU functionality for SQLite persistence. */ class SQLiteLruReferenceDelegate implements ReferenceDelegate, LruDelegate { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java index 496c95eacb2..571b302136a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java @@ -19,7 +19,6 @@ import android.database.sqlite.SQLiteStatement; import android.util.SparseArray; - import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; @@ -28,7 +27,6 @@ import com.google.firebase.firestore.proto.Target; import com.google.firebase.firestore.util.Consumer; import com.google.protobuf.InvalidProtocolBufferException; -import java.util.Set; import javax.annotation.Nullable; /** Cached Queries backed by SQLite. */ diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java index 19f51c94db7..11cbc6a3493 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java @@ -14,8 +14,6 @@ package com.google.firebase.firestore.local; -import android.util.SparseArray; - import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.keySet; import static com.google.firebase.firestore.testutil.TestUtil.query; @@ -28,6 +26,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import android.util.SparseArray; import com.google.firebase.Timestamp; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.ListenSequence;