Skip to content

Index-Free: Using readTime instead of updateTime #686

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 2 commits into from
Aug 7, 2019
Merged

Index-Free: Using readTime instead of updateTime #686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e53f1b5
Index-Free: Using readTime instead of updateTime
schmidt-sebastian Aug 5, 2019
152f465
Address comments
schmidt-sebastian Aug 7, 2019
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/LocalStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ public ImmutableSortedMap<DocumentKey, MaybeDocument> applyRemoteEvent(RemoteEve
|| doc.getVersion().equals(SnapshotVersion.NONE)
|| (authoritativeUpdates.contains(doc.getKey()) && !existingDoc.hasPendingWrites())
|| doc.getVersion().compareTo(existingDoc.getVersion()) >= 0) {
remoteDocuments.add(doc);
remoteDocuments.add(doc, remoteEvent.getSnapshotVersion());
changedDocs.put(key, doc);
} else {
Logger.debug(
Expand Down Expand Up @@ -634,7 +634,7 @@ private void applyWriteToRemoteDocuments(MutationBatchResult batchResult) {
batch,
remoteDoc);
} else {
remoteDocuments.add(doc);
remoteDocuments.add(doc, batchResult.getCommitVersion());
}
}
}
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 @@ -117,8 +117,8 @@ public int removeTargets(long upperBound, SparseArray<?> activeTargetIds) {
public int removeOrphanedDocuments(long upperBound) {
int count = 0;
MemoryRemoteDocumentCache cache = persistence.getRemoteDocumentCache();
for (Map.Entry<DocumentKey, MaybeDocument> entry : cache.getDocuments()) {
DocumentKey key = entry.getKey();
for (MaybeDocument doc : cache.getDocuments()) {
DocumentKey key = doc.getKey();
if (!isPinned(key, upperBound)) {
cache.remove(key);
orphanedSequenceNumbers.remove(key);
Expand Down
57 changes: 42 additions & 15 deletions ...irestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,38 +15,40 @@
package com.google.firebase.firestore.local;

import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap;
import static com.google.firebase.firestore.model.DocumentCollections.emptyMaybeDocumentMap;
import static com.google.firebase.firestore.util.Assert.hardAssert;

import android.util.Pair;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import com.google.firebase.database.collection.ImmutableSortedMap;
import com.google.firebase.firestore.core.Query;
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.MaybeDocument;
import com.google.firebase.firestore.model.ResourcePath;
import com.google.firebase.firestore.model.SnapshotVersion;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;

/** In-memory cache of remote documents. */
final class MemoryRemoteDocumentCache implements RemoteDocumentCache {

/** Underlying cache of documents. */
private ImmutableSortedMap<DocumentKey, MaybeDocument> docs;
/** Underlying cache of documents and their read times. */
private ImmutableSortedMap<DocumentKey, Pair<MaybeDocument, SnapshotVersion>> docs;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the real only benefit of the changes in this file (and the additional memory-overhead) is that we can run the LocalStore tests with IndexFree queries both against the MemoryRemoteDocumentCache and the SQLiteRemoteDocumentCache. We need to match the filter logic to be able to compare row read counts.


private final MemoryPersistence persistence;
private StatsCollector statsCollector;

MemoryRemoteDocumentCache(MemoryPersistence persistence, StatsCollector statsCollector) {
docs = emptyMaybeDocumentMap();
docs = ImmutableSortedMap.Builder.emptyMap(DocumentKey.comparator());
this.statsCollector = statsCollector;
this.persistence = persistence;
}

@Override
public void add(MaybeDocument document) {
docs = docs.insert(document.getKey(), document);
public void add(MaybeDocument document, SnapshotVersion readTime) {
docs = docs.insert(document.getKey(), new Pair<>(document, readTime));

persistence.getIndexManager().addToCollectionParentIndex(document.getKey().getPath().popLast());
}
Expand All @@ -61,7 +63,8 @@ public void remove(DocumentKey key) {
@Override
public MaybeDocument get(DocumentKey key) {
statsCollector.recordRowsRead(STATS_TAG, 1);
return docs.get(key);
Pair<MaybeDocument, SnapshotVersion> entry = docs.get(key);
return entry != null ? entry.first : null;
}

@Override
Expand Down Expand Up @@ -89,12 +92,13 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
// we need to match the query against.
ResourcePath queryPath = query.getPath();
DocumentKey prefix = DocumentKey.fromPath(queryPath.append(""));
Iterator<Map.Entry<DocumentKey, MaybeDocument>> iterator = docs.iteratorFrom(prefix);
Iterator<Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>>> iterator =
docs.iteratorFrom(prefix);

int rowsRead = 0;

while (iterator.hasNext()) {
Map.Entry<DocumentKey, MaybeDocument> entry = iterator.next();
Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>> entry = iterator.next();

++rowsRead;

Expand All @@ -103,7 +107,7 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
break;
}

MaybeDocument maybeDoc = entry.getValue();
MaybeDocument maybeDoc = entry.getValue().first;
if (!(maybeDoc instanceof Document)) {
continue;
}
Expand All @@ -119,8 +123,8 @@ public ImmutableSortedMap<DocumentKey, Document> getAllDocumentsMatchingQuery(Qu
return result;
}

ImmutableSortedMap<DocumentKey, MaybeDocument> getDocuments() {
return docs;
Iterable<MaybeDocument> getDocuments() {
return new DocumentIterable();
}

/**
Expand All @@ -140,10 +144,33 @@ private static long getKeySize(DocumentKey key) {

long getByteSize(LocalSerializer serializer) {
long count = 0;
for (Map.Entry<DocumentKey, MaybeDocument> entry : docs) {
count += getKeySize(entry.getKey());
count += serializer.encodeMaybeDocument(entry.getValue()).getSerializedSize();
for (MaybeDocument doc : new DocumentIterable()) {
count += getKeySize(doc.getKey());
count += serializer.encodeMaybeDocument(doc).getSerializedSize();
}
return count;
}

/**
* A proxy that exposes an iterator over the current set of documents in the RemoteDocumentCache.
*/
private class DocumentIterable implements Iterable<MaybeDocument> {
@NonNull
@Override
public Iterator<MaybeDocument> iterator() {
Iterator<Map.Entry<DocumentKey, Pair<MaybeDocument, SnapshotVersion>>> iterator =
MemoryRemoteDocumentCache.this.docs.iterator();
return new Iterator<MaybeDocument>() {
@Override
public boolean hasNext() {
return iterator.hasNext();
}

@Override
public MaybeDocument next() {
return iterator.next().getValue().first;
}
};
}
}
}
4 changes: 3 additions & 1 deletion ...base-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.firebase.firestore.model.Document;
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.MaybeDocument;
import com.google.firebase.firestore.model.SnapshotVersion;
import java.util.Map;

/**
Expand All @@ -40,8 +41,9 @@ interface RemoteDocumentCache {
* for the key, it will be replaced.
*
* @param maybeDocument A Document or NoDocument to put in the cache.
* @param readTime The time at which the document was read or committed.
*/
void add(MaybeDocument maybeDocument);
void add(MaybeDocument maybeDocument, SnapshotVersion readTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question: why not make readTime a part of the MaybeDocument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the road I went down on at first - it generates a significant amount of changes to plumb through a value that is only ever read directly at source.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sort of surprised there are that many changes. Did you happen to commit it so I can see?

For example: all our tests could use the same snapshot for both versions by default and only specify a second timestamp when the difference really matters. That means nearly all tests would be unaffected.

Oh, maybe the issue is that the read_time comes from the TargetChange and is not intrinsically a part of the document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are at least two additional challenges:

  • If I add it to MaybeDocument, I have to change it for Document, UnknownDocument and NoDocument. At least for NoDocuments it is throwaway work as these are never written to the cache.
  • MaybeDoocument currently uses the generic term version. This works well with all subtypes, but it doesn't quite work if introduce another kind of version.

I never actually got as far as to see how we would construct the individual documents. By exposing the read time only as an input to the RemoteDocumentCache there are only two callsites that need changing, which won me over.


/** Removes the cached entry for the given key (no-op if no entry exists). */
void remove(DocumentKey documentKey);
Expand Down
7 changes: 4 additions & 3 deletions ...irestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.MaybeDocument;
import com.google.firebase.firestore.model.ResourcePath;
import com.google.firebase.firestore.model.SnapshotVersion;
import com.google.firebase.firestore.util.BackgroundQueue;
import com.google.firebase.firestore.util.Executors;
import com.google.protobuf.InvalidProtocolBufferException;
Expand All @@ -50,16 +51,16 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache {
}

@Override
public void add(MaybeDocument maybeDocument) {
public void add(MaybeDocument maybeDocument, SnapshotVersion readTime) {
String path = pathForKey(maybeDocument.getKey());
Timestamp timestamp = maybeDocument.getVersion().getTimestamp();
Timestamp timestamp = readTime.getTimestamp();
MessageLite message = serializer.encodeMaybeDocument(maybeDocument);

statsCollector.recordRowsWritten(STATS_TAG, 1);

db.execute(
"INSERT OR REPLACE INTO remote_documents "
+ "(path, update_time_seconds, update_time_nanos, contents) "
+ "(path, read_time_seconds, read_time_nanos, contents) "
+ "VALUES (?, ?, ?, ?)",
path,
timestamp.getSeconds(),
Expand Down
14 changes: 7 additions & 7 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ void runMigrations(int fromVersion, int toVersion) {
}

if (fromVersion < 9 && toVersion >= 9) {
addUpdateTime();
addReadTime();
}

/*
Expand Down Expand Up @@ -363,13 +363,13 @@ private void addSequenceNumber() {
}
}

private void addUpdateTime() {
if (!tableContainsColumn("remote_documents", "update_time_seconds")) {
private void addReadTime() {
if (!tableContainsColumn("remote_documents", "read_time_seconds")) {
hardAssert(
!tableContainsColumn("remote_documents", "update_time_nanos"),
"Table contained update_time_seconds, but is missing update_time_nanos");
db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_seconds INTEGER");
db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_nanos INTEGER");
!tableContainsColumn("remote_documents", "read_time_nanos"),
"Table contained read_time_nanos, but is missing read_time_seconds");
db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER");
db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER");
}
}

Expand Down
5 changes: 3 additions & 2 deletions ...e-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ public void setUp() {
}

private void addDocument(Document newDoc) {
remoteDocuments.add(newDoc);
// Use document version as read time as the IndexedQueryEngine does not rely on read time.
remoteDocuments.add(newDoc, newDoc.getVersion());
queryEngine.handleDocumentChange(
deletedDoc(newDoc.getKey().toString(), ORIGINAL_VERSION), newDoc);
}
Expand All @@ -85,7 +86,7 @@ private void removeDocument(Document oldDoc) {
}

private void updateDocument(Document oldDoc, Document newDoc) {
remoteDocuments.add(newDoc);
remoteDocuments.add(newDoc, newDoc.getVersion());
queryEngine.handleDocumentChange(oldDoc, newDoc);
}

Expand Down
4 changes: 2 additions & 2 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 @@ -139,7 +139,7 @@ private Document nextTestDocument() {

private Document cacheADocumentInTransaction() {
Document doc = nextTestDocument();
documentCache.add(doc);
documentCache.add(doc, doc.getVersion());
return doc;
}

Expand Down Expand Up @@ -554,7 +554,7 @@ public void testRemoveTargetsThenGC() {
SnapshotVersion newVersion = version(3);
Document doc =
new Document(middleDocToUpdate, newVersion, Document.DocumentState.SYNCED, testValue);
documentCache.add(doc);
documentCache.add(doc, newVersion);
updateTargetInTransaction(middleTarget);
});

Expand Down
14 changes: 9 additions & 5 deletions ...estore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.google.firebase.firestore.testutil.TestUtil.map;
import static com.google.firebase.firestore.testutil.TestUtil.path;
import static com.google.firebase.firestore.testutil.TestUtil.values;
import static com.google.firebase.firestore.testutil.TestUtil.version;
import static java.util.Arrays.asList;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotEquals;
Expand All @@ -32,6 +33,7 @@
import com.google.firebase.firestore.model.DocumentKey;
import com.google.firebase.firestore.model.MaybeDocument;
import com.google.firebase.firestore.model.NoDocument;
import com.google.firebase.firestore.model.SnapshotVersion;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -134,7 +136,7 @@ public void testSetAndReadLotsOfDocuments() {
public void testSetAndReadDeletedDocument() {
String path = "a/b";
NoDocument deletedDoc = deletedDoc(path, 42);
add(deletedDoc);
add(deletedDoc, version(42));
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these tests use identical versions for the update_time and read_time which means we could have gotten these reversed and won't see it. Could you add a test that verifies read time doesn't get crossed with update time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will become easier to add this test when #617 lands (which should be the next PR). I added a TODO for now and will resolve it when we gain the ability to exclude documents by read time.

assertEquals(deletedDoc, get(path));
}

Expand All @@ -144,7 +146,7 @@ public void testSetDocumentToNewValue() {
Document written = addTestDocumentAtPath(path);

Document newDoc = doc(path, 57, map("data", 5));
add(newDoc);
add(newDoc, version(57));

assertNotEquals(written, newDoc);
assertEquals(newDoc, get(path));
Expand Down Expand Up @@ -182,12 +184,14 @@ public void testDocumentsMatchingQuery() {

private Document addTestDocumentAtPath(String path) {
Document doc = doc(path, 42, map("data", 2));
add(doc);
add(doc, version(42));
return doc;
}

private void add(MaybeDocument doc) {
persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc));
// TODO(mrschmidt): Add a test uses different update and read times and verifies that we correctly
// filter by read time
private void add(MaybeDocument doc, SnapshotVersion readTime) {
persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc, readTime));
}

@Nullable
Expand Down