Skip to content

LocalStore uses a SparseArray to track target ids, not a set #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 9, 2018
Merged

LocalStore uses a SparseArray to track target ids, not a set #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4a5a21d
LocalStore uses a SparseArray to track target ids, not a set
Oct 8, 2018
92cd529
Add android stuff to memory version of the lru tests
Oct 8, 2018
0112fa7
Formatting
Oct 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/LruDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@

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
Expand All @@ -37,7 +37,7 @@ interface LruDelegate {
*
* @return the number of targets removed.
*/
int removeTargets(long upperBound, Set<Integer> activeTargetIds);
int removeTargets(long upperBound, SparseArray<?> activeTargetIds);

/**
* Removes all unreferenced documents from the cache that have a sequence number less than or
Expand Down
4 changes: 2 additions & 2 deletions ...base-firestore/src/main/java/com/google/firebase/firestore/local/LruGarbageCollector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@

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 {
Expand Down Expand Up @@ -80,7 +80,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<Integer> activeTargetIds) {
int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
return delegate.removeTargets(upperBound, activeTargetIds);
}

Expand Down
4 changes: 2 additions & 2 deletions ...restore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@

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 {
Expand Down Expand Up @@ -94,7 +94,7 @@ public void setInMemoryPins(ReferenceSet inMemoryPins) {
}

@Override
public int removeTargets(long upperBound, Set<Integer> activeTargetIds) {
public int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
return persistence.getQueryCache().removeQueries(upperBound, activeTargetIds);
}

Expand Down
6 changes: 3 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryQueryCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

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;
Expand All @@ -22,7 +23,6 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -115,13 +115,13 @@ public void removeQueryData(QueryData queryData) {
*
* @return the number of targets removed
*/
int removeQueries(long upperBound, Set<Integer> activeTargetIds) {
int removeQueries(long upperBound, SparseArray<?> activeTargetIds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in this function indicates to me that we always need to accept a SparseArray. For any other sparse array, activeTargetIds.get(targetId) would return null. Can you explain to me why you chose the wildcard in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, why would any other SparseArray return null? The target ids are the keys, and SparseArray is required to use integer keys. It doesn't matter what the value is, we are only ever using the keys. Because we don't care about the value, the ? is appropriate. It indicates we will not make use of the value, which we don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per our offline discussion, this is ok. It's just not the most obvious use of an array-like data structure to me. I wish it was possible to make the key-only nature more obvious (such as with a Map's keySet), but since that's not possible without a copy, this LGTM

int removed = 0;
for (Iterator<Map.Entry<Query, QueryData>> it = queries.entrySet().iterator(); it.hasNext(); ) {
Map.Entry<Query, QueryData> 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++;
Expand Down
4 changes: 2 additions & 2 deletions ...restore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

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 {
Expand Down Expand Up @@ -103,7 +103,7 @@ public void removeReference(DocumentKey key) {
}

@Override
public int removeTargets(long upperBound, Set<Integer> activeTargetIds) {
public int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
return persistence.getQueryCache().removeQueries(upperBound, activeTargetIds);
}

Expand Down
6 changes: 3 additions & 3 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteQueryCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
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;
Expand All @@ -26,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. */
Expand Down Expand Up @@ -186,7 +186,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<Integer> 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
Expand All @@ -196,7 +196,7 @@ int removeQueries(long upperBound, Set<Integer> activeTargetIds) {
.forEach(
row -> {
int targetId = row.getInt(0);
if (!activeTargetIds.contains(targetId)) {
if (activeTargetIds.get(targetId) == null) {
removeTarget(targetId);
count[0]++;
}
Expand Down
11 changes: 6 additions & 5 deletions ...estore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,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;
Expand Down Expand Up @@ -162,7 +163,7 @@ private void removeDocumentFromTarget(DocumentKey key, int targetId) {
queryCache.removeMatchingKeys(keySet(key), targetId);
}

private int removeTargets(long upperBound, Set<Integer> activeTargetIds) {
private int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
return persistence.runTransaction(
"Remove queries", () -> garbageCollector.removeTargets(upperBound, activeTargetIds));
}
Expand Down Expand Up @@ -283,7 +284,7 @@ public void testSequenceNumbersWithMutationsInQueries() {

@Test
public void testRemoveQueriesUpThroughSequenceNumber() {
Map<Integer, QueryData> activeTargetIds = new HashMap<>();
SparseArray<QueryData> 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.
Expand All @@ -295,7 +296,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(
Expand Down Expand Up @@ -567,8 +568,8 @@ public void testRemoveTargetsThenGC() {
});

// Finally, do the garbage collection, up to but not including the removal of middleTarget
Set<Integer> activeTargetIds = new HashSet<>();
activeTargetIds.add(oldestTarget.getTargetId());
SparseArray<QueryData> activeTargetIds = new SparseArray<>();
activeTargetIds.put(oldestTarget.getTargetId(), oldestTarget);
int targetsRemoved = garbageCollector.removeTargets(upperBound, activeTargetIds);
// Expect to remove newest target
assertEquals(1, targetsRemoved);
Expand Down
6 changes: 6 additions & 0 deletions ...tore/src/test/java/com/google/firebase/firestore/local/MemoryLruGarbageCollectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down