diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 091ad59233b..d598636cc42 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,5 @@ # Unreleased - +* [changed] Improve efficiency of memory persistence when processing a large number of writes. [#6233](//github.com/firebase/firebase-android-sdk/pull/6233) # 25.1.0 * [feature] Add support for the VectorValue type. [#6154](//github.com/firebase/firebase-android-sdk/pull/6154) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 9d268a503ba..aa9275e0e45 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -208,17 +208,17 @@ public LocalDocumentsView getLocalDocumentsForCurrentUser() { public ImmutableSortedMap handleUserChange(User user) { // Swap out the mutation queue, grabbing the pending mutation batches before and after. - List oldBatches = mutationQueue.getAllMutationBatches(); + Collection oldBatches = mutationQueue.getAllMutationBatches(); initializeUserComponents(user); startIndexManager(); startMutationQueue(); - List newBatches = mutationQueue.getAllMutationBatches(); + Collection newBatches = mutationQueue.getAllMutationBatches(); // Union the old/new changed keys. ImmutableSortedSet changedKeys = DocumentKey.emptyKeySet(); - for (List batches : asList(oldBatches, newBatches)) { + for (Collection batches : asList(oldBatches, newBatches)) { for (MutationBatch batch : batches) { for (Mutation mutation : batch.getMutations()) { changedKeys = changedKeys.insert(mutation.getKey()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index 7a238dd20c1..3a2cd1541c9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -31,9 +31,12 @@ import com.google.firebase.firestore.util.Util; import com.google.protobuf.ByteString; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.Map; +import java.util.TreeMap; final class MemoryMutationQueue implements MutationQueue { @@ -54,7 +57,7 @@ final class MemoryMutationQueue implements MutationQueue { *

Once the held write acknowledgements become visible they are removed from the head of the * queue along with any tombstones that follow. */ - private final List queue; + private final TreeMap queue = new TreeMap<>(); /** An ordered mapping between documents and the mutation batch IDs. */ private ImmutableSortedSet batchesByDocumentKey; @@ -74,7 +77,6 @@ final class MemoryMutationQueue implements MutationQueue { MemoryMutationQueue(MemoryPersistence persistence, User user) { this.persistence = persistence; - queue = new ArrayList<>(); batchesByDocumentKey = new ImmutableSortedSet<>(emptyList(), DocumentReference.BY_KEY); nextBatchId = 1; @@ -105,16 +107,20 @@ public boolean isEmpty() { @Override public void acknowledgeBatch(MutationBatch batch, ByteString streamToken) { int batchId = batch.getBatchId(); - int batchIndex = indexOfExistingBatchId(batchId, "acknowledged"); - hardAssert(batchIndex == 0, "Can only acknowledge the first batch in the mutation queue"); + hardAssert( + queue.containsKey(batchId), + "Can not acknowledge unknown batch ID: %d (batch=%s)", + batchId, + batch); - // Verify that the batch in the queue is the one to be acknowledged. - MutationBatch check = queue.get(batchIndex); + int firstBatchId = queue.firstKey(); hardAssert( - batchId == check.getBatchId(), - "Queue ordering failure: expected batch %d, got batch %d", + batchId == firstBatchId, + "Can only acknowledge the first batch in the mutation queue:" + + " got batchId %d, but expected %d (batch=%s)", batchId, - check.getBatchId()); + firstBatchId, + batch); lastStreamToken = checkNotNull(streamToken); } @@ -137,15 +143,17 @@ public MutationBatch addMutationBatch( int batchId = nextBatchId; nextBatchId += 1; - int size = queue.size(); - if (size > 0) { - MutationBatch prior = queue.get(size - 1); + if (!queue.isEmpty()) { + int priorKey = queue.lastKey(); hardAssert( - prior.getBatchId() < batchId, "Mutation batchIds must be monotonically increasing order"); + priorKey < batchId, + "Mutation batchIds must be monotonically increasing order: priorKey=%d, batchId=%d", + priorKey, + batchId); } MutationBatch batch = new MutationBatch(batchId, localWriteTime, baseMutations, mutations); - queue.add(batch); + queue.put(batchId, batch); // Track references by document key and index collection parents. for (Mutation mutation : mutations) { @@ -161,25 +169,14 @@ public MutationBatch addMutationBatch( @Nullable @Override public MutationBatch lookupMutationBatch(int batchId) { - int index = indexOfBatchId(batchId); - if (index < 0 || index >= queue.size()) { - return null; - } - - MutationBatch batch = queue.get(index); - hardAssert(batch.getBatchId() == batchId, "If found batch must match"); - return batch; + return queue.get(batchId); } @Nullable @Override public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { - int nextBatchId = batchId + 1; - - // The requested batchId may still be out of range so normalize it to the start of the queue. - int rawIndex = indexOfBatchId(nextBatchId); - int index = rawIndex < 0 ? 0 : rawIndex; - return queue.size() > index ? queue.get(index) : null; + Map.Entry nextEntry = queue.higherEntry(batchId); + return nextEntry == null ? null : nextEntry.getValue(); } @Override @@ -188,8 +185,8 @@ public int getHighestUnacknowledgedBatchId() { } @Override - public List getAllMutationBatches() { - return Collections.unmodifiableList(queue); + public Collection getAllMutationBatches() { + return Collections.unmodifiableCollection(queue.values()); } @Override @@ -292,12 +289,23 @@ private List lookupMutationBatches(ImmutableSortedSet ba @Override public void removeMutationBatch(MutationBatch batch) { - // Find the position of the first batch for removal. This need not be the first entry in the - // queue. - int batchIndex = indexOfExistingBatchId(batch.getBatchId(), "removed"); - hardAssert(batchIndex == 0, "Can only remove the first entry of the mutation queue"); + int batchId = batch.getBatchId(); + hardAssert( + queue.containsKey(batchId), + "Can not remove unknown batch ID: %d (batch=%s)", + batchId, + batch); + + int firstBatchId = queue.firstKey(); + hardAssert( + batchId == firstBatchId, + "Can only remove the first batch in the mutation queue:" + + " got batchId %d, but expected %d (batch=%s)", + batchId, + firstBatchId, + batch); - queue.remove(0); + queue.remove(batchId); // Remove entries from the index too. ImmutableSortedSet references = batchesByDocumentKey; @@ -336,45 +344,9 @@ boolean containsKey(DocumentKey key) { // Helpers - /** - * Finds the index of the given batchId in the mutation queue. This operation is O(1). - * - * @return The computed index of the batch with the given batchId, based on the state of the - * queue. Note this index can be negative if the requested batchId has already been removed - * from the queue or past the end of the queue if the batchId is larger than the last added - * batch. - */ - private int indexOfBatchId(int batchId) { - if (queue.isEmpty()) { - // As an index this is past the end of the queue - return 0; - } - - // Examine the front of the queue to figure out the difference between the batchId and indexes - // in the array. Note that since the queue is ordered by batchId, if the first batch has a - // larger batchId then the requested batchId doesn't exist in the queue. - MutationBatch firstBatch = queue.get(0); - int firstBatchId = firstBatch.getBatchId(); - return batchId - firstBatchId; - } - - /** - * Finds the index of the given batchId in the mutation queue and asserts that the resulting index - * is within the bounds of the queue. - * - * @param batchId The batchId to search for - * @param action A description of what the caller is doing, phrased in passive form (for example - * "acknowledged" in a routine that acknowledges batches). - */ - private int indexOfExistingBatchId(int batchId, String action) { - int index = indexOfBatchId(batchId); - hardAssert(index >= 0 && index < queue.size(), "Batches must exist to be %s", action); - return index; - } - long getByteSize(LocalSerializer serializer) { long count = 0; - for (MutationBatch batch : queue) { + for (MutationBatch batch : queue.values()) { count += serializer.encodeMutationBatch(batch).getSerializedSize(); } return count; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index 02a7f7ea359..c3e3fa47949 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -21,6 +21,7 @@ import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatch; import com.google.protobuf.ByteString; +import java.util.Collection; import java.util.List; /** A queue of mutations to apply to the remote store. */ @@ -79,7 +80,7 @@ MutationBatch addMutationBatch( /** Returns all mutation batches in the mutation queue. */ // TODO: PERF: Current consumer only needs mutated keys; if we can provide that // cheaply, we should replace this. - List getAllMutationBatches(); + Collection getAllMutationBatches(); /** * Finds all mutation batches that could @em possibly affect the given document key. Not all diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteOverlayMigrationManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteOverlayMigrationManager.java index 58ea1a60456..43651614ae2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteOverlayMigrationManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteOverlayMigrationManager.java @@ -20,8 +20,8 @@ import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.mutation.MutationBatch; +import java.util.Collection; import java.util.HashSet; -import java.util.List; import java.util.Set; /** Manages overlay migrations required to have overlay support . */ @@ -58,7 +58,7 @@ private void buildOverlays() { // Get all document keys that have local mutations Set allDocumentKeys = new HashSet<>(); - List batches = mutationQueue.getAllMutationBatches(); + Collection batches = mutationQueue.getAllMutationBatches(); for (MutationBatch batch : batches) { allDocumentKeys.addAll(batch.getKeys()); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java index 132c85619b9..e3ae2813f56 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MutationQueueTestCase.java @@ -40,6 +40,7 @@ import com.google.firebase.firestore.remote.WriteStream; import com.google.protobuf.ByteString; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; @@ -343,10 +344,10 @@ public void testRemoveMutationBatches() { removeMutationBatches(batches.remove(0)); assertEquals(9, batchCount()); - List found; + Collection found; found = mutationQueue.getAllMutationBatches(); - assertEquals(batches, found); + assertEquals(batches, new ArrayList<>(found)); assertEquals(9, found.size()); removeMutationBatches(batches.get(0), batches.get(1), batches.get(2)); @@ -356,14 +357,14 @@ public void testRemoveMutationBatches() { assertEquals(6, batchCount()); found = mutationQueue.getAllMutationBatches(); - assertEquals(batches, found); + assertEquals(batches, new ArrayList<>(found)); assertEquals(6, found.size()); removeMutationBatches(batches.remove(0)); assertEquals(5, batchCount()); found = mutationQueue.getAllMutationBatches(); - assertEquals(batches, found); + assertEquals(batches, new ArrayList<>(found)); assertEquals(5, found.size()); removeMutationBatches(batches.remove(0)); @@ -373,12 +374,12 @@ public void testRemoveMutationBatches() { assertEquals(3, batchCount()); found = mutationQueue.getAllMutationBatches(); - assertEquals(batches, found); + assertEquals(batches, new ArrayList<>(found)); assertEquals(3, found.size()); removeMutationBatches(batches.toArray(new MutationBatch[0])); found = mutationQueue.getAllMutationBatches(); - assertEquals(emptyList(), found); + assertEquals(emptyList(), new ArrayList<>(found)); assertEquals(0, found.size()); assertTrue(mutationQueue.isEmpty()); }