Skip to content

Port performance optimizations to speed up reading large collections from Android #1433

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.

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 30 commits into from
Dec 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3da020e
1
var-const Dec 14, 2018
9717fef
Compiles
var-const Dec 14, 2018
204443e
Compiles, pt 2
var-const Dec 14, 2018
c56820b
Compiles, pt 3m
var-const Dec 14, 2018
f3174ca
[AUTOMATED]: Prettier Code Styling
var-const Dec 14, 2018
36c53d3
Some fixes
var-const Dec 14, 2018
942b0fa
Very hacky version works
var-const Dec 15, 2018
7767b60
Most unit tests pass
var-const Dec 17, 2018
a1ad6d2
[AUTOMATED]: Prettier Code Styling
var-const Dec 17, 2018
cc29731
Undo temp/accidental changes
var-const Dec 17, 2018
307e0b3
applyRemoteEvent, sized entries
var-const Dec 18, 2018
62c627b
Fix failing tests
var-const Dec 18, 2018
8f293eb
Serializer
var-const Dec 18, 2018
4e23f3b
[AUTOMATED]: Prettier Code Styling
var-const Dec 18, 2018
fb751dd
small cleanup
var-const Dec 18, 2018
431f618
Fix accidental
var-const Dec 18, 2018
1681f3d
Comment
var-const Dec 18, 2018
55c7ff3
Review feedback 1, test
var-const Dec 19, 2018
5afa305
[AUTOMATED]: Prettier Code Styling
var-const Dec 19, 2018
56eefbc
Review feedback 1
var-const Dec 20, 2018
5e506fa
Review feedback 2
var-const Dec 20, 2018
7b11cec
[AUTOMATED]: Prettier Code Styling
var-const Dec 20, 2018
1a32b22
Review feedback 3
var-const Dec 21, 2018
17a69a6
Review feedback
var-const Dec 22, 2018
4c270c7
Appease linter
var-const Dec 22, 2018
512667e
Fix node tests
var-const Dec 22, 2018
6cb4bfb
Appease linter 2
var-const Dec 22, 2018
77bf92e
[AUTOMATED]: Prettier Code Styling
var-const Dec 22, 2018
e7b8c8e
Comment
var-const Dec 22, 2018
a0d25f5
Merge branch 'master' into varconst/port-android-1000-reads-3
var-const Dec 22, 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
5 changes: 4 additions & 1 deletion packages/firestore/src/local/indexeddb_mutation_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { BATCHID_UNKNOWN, MutationBatch } from '../model/mutation_batch';
import { ResourcePath } from '../model/path';
import { assert, fail } from '../util/assert';
import { primitiveComparator } from '../util/misc';
import { SortedMap } from '../util/sorted_map';
import { SortedSet } from '../util/sorted_set';

import * as EncodedResourcePath from './encoded_resource_path';
Expand All @@ -46,6 +47,8 @@ import { PersistenceTransaction, ReferenceDelegate } from './persistence';
import { PersistencePromise } from './persistence_promise';
import { SimpleDbStore, SimpleDbTransaction } from './simple_db';

import { AnyJs } from '../../src/util/misc';

/** A mutation queue for a specific user, backed by IndexedDB. */
export class IndexedDbMutationQueue implements MutationQueue {
/**
Expand Down Expand Up @@ -325,7 +328,7 @@ export class IndexedDbMutationQueue implements MutationQueue {

getAllMutationBatchesAffectingDocumentKeys(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
documentKeys: SortedMap<DocumentKey, AnyJs>
): PersistencePromise<MutationBatch[]> {
let uniqueBatchIDs = new SortedSet<BatchId>(primitiveComparator);

Expand Down
118 changes: 117 additions & 1 deletion packages/firestore/src/local/indexeddb_remote_document_cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@

import { Query } from '../core/query';
import {
DocumentKeySet,
documentKeySet,
DocumentMap,
documentMap,
DocumentSizeEntries,
DocumentSizeEntry,
MaybeDocumentMap,
maybeDocumentMap
maybeDocumentMap,
nullableMaybeDocumentMap,
NullableMaybeDocumentMap
} from '../model/collections';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { SortedMap } from '../util/sorted_map';

import { SnapshotVersion } from '../core/snapshot_version';
import { assert, fail } from '../util/assert';
Expand Down Expand Up @@ -178,6 +183,110 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
});
}

getEntries(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
): PersistencePromise<NullableMaybeDocumentMap> {
let results = nullableMaybeDocumentMap();
return this.forEachDbEntry(
transaction,
documentKeys,
(key, dbRemoteDoc) => {
if (dbRemoteDoc) {
results = results.insert(
key,
this.serializer.fromDbRemoteDocument(dbRemoteDoc)
);
} else {
results = results.insert(key, null);
}
}
).next(() => results);
}

/**
* Looks up several entries in the cache.
*
* @param documentKeys The set of keys entries to look up.
* @return A map of MaybeDocuments indexed by key (if a document cannot be
* found, the key will be mapped to null) and a map of sizes indexed by
* key (zero if the key cannot be found).
*/
getSizedEntries(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another approach would be for getSizedEntries to return a SortedMap<DocumentKey, DocumentSizeEntry>. I decided in favor of returning two maps because it makes it easier to avoid code duplication between getEntries and getSizedEntries.

Copy link

Choose a reason for hiding this comment

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

Consider taking as an argument a function that processes each key into the type of the result, for example fn: (key: DocumentKey, doc: DbRemoteDoc | null) => T. Then, your return type can be PersistencePromise<SortedMap<DocumentKey, T>>. You can avoid code duplication and avoid doing extra work for sizes that way.

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 tried it out, but I'm not sure I prefer it. The problem is that getEntries in RemoteDocumentChangeBuffer won't be able to return a map of documents directly (due to type difference) and instead would have to build a new map. If extra work for calculating/storing sizes is a concern, it's easy (though ugly) to solve with a flag (or perhaps, more similar to this approach, by having a fn that either updates the sizeMap or is a no-op).

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like that makes sense, but since it deals with DbRemoteDoc, I'd keep it internal and have getEntries() and getSizedEntries() functions that wrap it.

I think what Greg is recommending is basically a mapDbEntries() function, but it might be a little simpler to instead have it be a forEachDbEntry(transaction, documentKeys, callback) function that iterates the matching documents and just calls the callback with each raw dbRemoteDocument. That may mean a little bit of redundant code for getEntries() and getSizedEntries() to build up their respective maps, but it seems simpler to me (and perhaps more generically useful, if we had a case where we don't necessarily want to build up a map).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please take a look.

transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
): PersistencePromise<DocumentSizeEntries> {
let results = nullableMaybeDocumentMap();
let sizeMap = new SortedMap<DocumentKey, number>(DocumentKey.comparator);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, now that getEntries() is no longer a wrapper around getSizedEntries(), can we drop sizeMap and have this be new SortedMap<DocumentKey, DocumentSizeEntry|null> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, I don't feel strongly about this. The reason I set it up that way is so that getEntries in RemoteDocumentChangeBuffer can return the MaybeDocumentMap directly. If this were a sorted map of DocumentSizeEntrys, then getEntries would have to create a new MaybeDocumentMap and fill it with just MaybeDocuments.

It's probably not a big deal, so if you think code clarity is more important here, I'll do the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry! I was confused. I thought it was just so the old getEntries() implementation could return the MaybeDocumentMap directly. But I see now that RemoteDocumentChangeBuffer.getEntries() ends up calling getSizedEntries() and using the sizes and also passing the MaybeDocumentMap straight through. So it needs both, and the way it's structured right now makes sense.

So nevermind. Please keep it the way it is.

return this.forEachDbEntry(
transaction,
documentKeys,
(key, dbRemoteDoc) => {
if (dbRemoteDoc) {
results = results.insert(
key,
this.serializer.fromDbRemoteDocument(dbRemoteDoc)
);
sizeMap = sizeMap.insert(key, dbDocumentSize(dbRemoteDoc));
} else {
results = results.insert(key, null);
sizeMap = sizeMap.insert(key, 0);
}
}
).next(() => {
return { maybeDocuments: results, sizeMap };
});
}

private forEachDbEntry(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet,
callback: (key: DocumentKey, doc: DbRemoteDocument | null) => void
): PersistencePromise<void> {
if (documentKeys.isEmpty()) {
return PersistencePromise.resolve();
}

const range = IDBKeyRange.bound(
documentKeys.first()!.path.toArray(),
documentKeys.last()!.path.toArray()
);
const keyIter = documentKeys.getIterator();
let nextKey: DocumentKey | null = keyIter.getNext();

return remoteDocumentsStore(transaction)
.iterate({ range }, (potentialKeyRaw, dbRemoteDoc, control) => {
const potentialKey = DocumentKey.fromSegments(potentialKeyRaw);

// Go through keys not found in cache.
while (nextKey && DocumentKey.comparator(nextKey!, potentialKey) < 0) {
callback(nextKey!, null);
nextKey = keyIter.getNext();
}

if (nextKey && nextKey!.isEqual(potentialKey)) {
// Key found in cache.
callback(nextKey!, dbRemoteDoc);
nextKey = keyIter.hasNext() ? keyIter.getNext() : null;
}

// Skip to the next key (if there is one).
if (nextKey) {
control.skip(nextKey!.path.toArray());
} else {
control.done();
}
})
.next(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment.

// The rest of the keys must not be in the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (with minor addition).

// The rest of the keys are not in the cache. One case where `iterate`
// above won't go through them is when the cache is empty.
while (nextKey) {
callback(nextKey!, null);
nextKey = keyIter.hasNext() ? keyIter.getNext() : null;
}
});
}

getDocumentsMatchingQuery(
transaction: PersistenceTransaction,
query: Query
Expand Down Expand Up @@ -381,6 +490,13 @@ class IndexedDbRemoteDocumentChangeBuffer extends RemoteDocumentChangeBuffer {
): PersistencePromise<DocumentSizeEntry | null> {
return this.documentCache.getSizedEntry(transaction, documentKey);
}

protected getAllFromCache(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
): PersistencePromise<DocumentSizeEntries> {
return this.documentCache.getSizedEntries(transaction, documentKeys);
}
}

export function isDocumentChangeMissingError(err: FirestoreError): boolean {
Expand Down
66 changes: 47 additions & 19 deletions packages/firestore/src/local/local_documents_view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import {
DocumentMap,
documentMap,
MaybeDocumentMap,
maybeDocumentMap
maybeDocumentMap,
NullableMaybeDocumentMap,
nullableMaybeDocumentMap
} from '../model/collections';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
Expand Down Expand Up @@ -74,6 +76,23 @@ export class LocalDocumentsView {
});
}

// Returns the view of the given `docs` as they would appear after applying
// all mutations in the given `batches`.
private applyLocalMutationsToDocuments(
transaction: PersistenceTransaction,
docs: NullableMaybeDocumentMap,
batches: MutationBatch[]
): NullableMaybeDocumentMap {
let results = nullableMaybeDocumentMap();
docs.forEach((key, localView) => {
for (const batch of batches) {
localView = batch.applyToLocalView(key, localView);
}
results = results.insert(key, localView);
});
return results;
}

/**
* Gets the local view of the documents identified by `keys`.
*
Expand All @@ -83,29 +102,38 @@ export class LocalDocumentsView {
getDocuments(
transaction: PersistenceTransaction,
keys: DocumentKeySet
): PersistencePromise<MaybeDocumentMap> {
return this.remoteDocumentCache
.getEntries(transaction, keys)
.next(docs => this.getLocalViewOfDocuments(transaction, docs));
}

/**
* Similar to `getDocuments`, but creates the local view from the given
* `baseDocs` without retrieving documents from the local store.
*/
getLocalViewOfDocuments(
transaction: PersistenceTransaction,
baseDocs: NullableMaybeDocumentMap
): PersistencePromise<MaybeDocumentMap> {
return this.mutationQueue
.getAllMutationBatchesAffectingDocumentKeys(transaction, keys)
.getAllMutationBatchesAffectingDocumentKeys(transaction, baseDocs)
.next(batches => {
const promises = [] as Array<PersistencePromise<void>>;
const docs = this.applyLocalMutationsToDocuments(
transaction,
baseDocs,
batches
);
let results = maybeDocumentMap();
keys.forEach(key => {
promises.push(
this.getDocumentInternal(transaction, key, batches).next(
maybeDoc => {
// TODO(http://b/32275378): Don't conflate missing / deleted.
if (!maybeDoc) {
maybeDoc = new NoDocument(
key,
SnapshotVersion.forDeletedDoc()
);
}
results = results.insert(key, maybeDoc);
}
)
);
docs.forEach((key, maybeDoc) => {
// TODO(http://b/32275378): Don't conflate missing / deleted.
if (!maybeDoc) {
maybeDoc = new NoDocument(key, SnapshotVersion.forDeletedDoc());
}
results = results.insert(key, maybeDoc);
});
return PersistencePromise.waitFor(promises).next(() => results);

return results;
});
}

Expand Down
4 changes: 3 additions & 1 deletion packages/firestore/src/local/local_serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ export class LocalSerializer {
/** Encodes a document for storage locally. */
toDbRemoteDocument(maybeDoc: MaybeDocument): DbRemoteDocument {
if (maybeDoc instanceof Document) {
const doc = this.remoteSerializer.toDocument(maybeDoc);
const doc = maybeDoc.proto
? maybeDoc.proto
: this.remoteSerializer.toDocument(maybeDoc);
const hasCommittedMutations = maybeDoc.hasCommittedMutations;
return new DbRemoteDocument(
/* unknownDocument= */ null,
Expand Down
42 changes: 29 additions & 13 deletions packages/firestore/src/local/local_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
DocumentKeySet,
documentKeySet,
DocumentMap,
maybeDocumentMap,
MaybeDocumentMap
} from '../model/collections';
import { MaybeDocument } from '../model/document';
Expand Down Expand Up @@ -466,11 +467,18 @@ export class LocalStore {
}
);

let changedDocKeys = documentKeySet();
let changedDocs = maybeDocumentMap();
let updatedKeys = documentKeySet();
remoteEvent.documentUpdates.forEach((key, doc) => {
changedDocKeys = changedDocKeys.add(key);
promises.push(
documentBuffer.getEntry(txn, key).next(existingDoc => {
updatedKeys = updatedKeys.add(key);
});

// Each loop iteration only affects its "own" doc, so it's safe to get all the remote
// documents in advance in a single call.
promises.push(
documentBuffer.getEntries(txn, updatedKeys).next(existingDocs => {
remoteEvent.documentUpdates.forEach((key, doc) => {
const existingDoc = existingDocs.get(key);
// If a document update isn't authoritative, make sure we don't
// apply an old document version to the remote cache. We make an
// exception for SnapshotVersion.MIN which can happen for
Expand All @@ -484,6 +492,7 @@ export class LocalStore {
doc.version.compareTo(existingDoc.version) >= 0
) {
documentBuffer.addEntry(doc);
changedDocs = changedDocs.insert(key, doc);
} else {
log.debug(
LOG_TAG,
Expand All @@ -495,14 +504,18 @@ export class LocalStore {
doc.version
);
}
})
);
if (remoteEvent.resolvedLimboDocuments.has(key)) {
promises.push(
this.persistence.referenceDelegate.updateLimboDocument(txn, key)
);
}
});

if (remoteEvent.resolvedLimboDocuments.has(key)) {
promises.push(
this.persistence.referenceDelegate.updateLimboDocument(
txn,
key
)
);
}
});
})
);

// HACK: The only reason we allow a null snapshot version is so that we
// can synthesize remote events when we get permission denied errors while
Expand Down Expand Up @@ -532,7 +545,10 @@ export class LocalStore {
return PersistencePromise.waitFor(promises)
.next(() => documentBuffer.apply(txn))
.next(() => {
return this.localDocuments.getDocuments(txn, changedDocKeys);
return this.localDocuments.getLocalViewOfDocuments(
txn,
changedDocs
);
});
}
);
Expand Down
5 changes: 4 additions & 1 deletion packages/firestore/src/local/memory_mutation_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,16 @@ import { BATCHID_UNKNOWN, MutationBatch } from '../model/mutation_batch';
import { emptyByteString } from '../platform/platform';
import { assert } from '../util/assert';
import { primitiveComparator } from '../util/misc';
import { SortedMap } from '../util/sorted_map';
import { SortedSet } from '../util/sorted_set';

import { MutationQueue } from './mutation_queue';
import { PersistenceTransaction, ReferenceDelegate } from './persistence';
import { PersistencePromise } from './persistence_promise';
import { DocReference } from './reference_set';

import { AnyJs } from '../../src/util/misc';

export class MemoryMutationQueue implements MutationQueue {
/**
* The set of all mutations that have been sent but not yet been applied to
Expand Down Expand Up @@ -203,7 +206,7 @@ export class MemoryMutationQueue implements MutationQueue {

getAllMutationBatchesAffectingDocumentKeys(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
documentKeys: SortedMap<DocumentKey, AnyJs>
): PersistencePromise<MutationBatch[]> {
let uniqueBatchIDs = new SortedSet<number>(primitiveComparator);

Expand Down
Loading