Skip to content

Port optimizations to LocalDocumentsView from iOS #1055

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 22 commits into from
Jul 31, 2018
Merged

Port optimizations to LocalDocumentsView from iOS #1055

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
65a53b2
wip
var-const Jul 25, 2018
f94e186
Local docs, path
var-const Jul 25, 2018
64a293d
memory mutation queue, test left
var-const Jul 25, 2018
a969f3b
test fix
var-const Jul 26, 2018
307a478
Fix iteration
var-const Jul 27, 2018
2de3301
add test
var-const Jul 27, 2018
5bf87ae
[AUTOMATED]: Prettier Code Styling
var-const Jul 27, 2018
90a04cb
small fixes
var-const Jul 27, 2018
b94e7c7
appease linter
var-const Jul 27, 2018
5b24623
Initial
var-const Jul 27, 2018
49a261e
[AUTOMATED]: Prettier Code Styling
var-const Jul 27, 2018
27c88eb
Merge branch 'varconst/silence-timestamp-warning-in-tests' into varco…
var-const Jul 27, 2018
223c430
review feedback wip
var-const Jul 28, 2018
42bf72b
Merge branch 'master' into varconst/port-affecting-batches
var-const Jul 29, 2018
c8375a6
Review feedback, indexeddb part
var-const Jul 29, 2018
87e0b06
Review feedback, memory part
var-const Jul 29, 2018
7082064
Merge branch 'master' into varconst/port-affecting-batches
var-const Jul 31, 2018
0d068f5
Review fix
var-const Jul 31, 2018
5dad79f
[AUTOMATED]: Prettier Code Styling
var-const Jul 31, 2018
ac1a01b
Merge branch 'master' into varconst/port-affecting-batches
var-const Jul 31, 2018
c40e50f
Merge branch 'varconst/port-affecting-batches' of github.com:firebase…
var-const Jul 31, 2018
e6cf4f5
Merge branch 'master' into varconst/port-affecting-batches
var-const Jul 31, 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
89 changes: 65 additions & 24 deletions packages/firestore/src/local/indexeddb_mutation_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ import { Timestamp } from '../api/timestamp';
import { User } from '../auth/user';
import { Query } from '../core/query';
import { BatchId, ProtoByteString } from '../core/types';
import { DocumentKeySet } from '../model/collections';
import { DocumentKey } from '../model/document_key';
import { Mutation } from '../model/mutation';
import { BATCHID_UNKNOWN, MutationBatch } from '../model/mutation_batch';
import { ResourcePath } from '../model/path';
import { Path, ResourcePath } from '../model/path';
import { assert, fail } from '../util/assert';
import { immediatePredecessor, primitiveComparator } from '../util/misc';
import { SortedSet } from '../util/sorted_set';
Expand Down Expand Up @@ -362,6 +363,41 @@ export class IndexedDbMutationQueue implements MutationQueue {
.next(() => results);
}

getAllMutationBatchesAffectingDocumentKeys(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
): PersistencePromise<MutationBatch[]> {
if (documentKeys.isEmpty()) {
return PersistencePromise.resolve([]);
}

const indexStart = DbDocumentMutation.prefixForPath(
this.userId,
documentKeys.first().path
);
const keyRange = IDBKeyRange.lowerBound(indexStart);
let uniqueBatchIDs = new SortedSet<BatchId>(primitiveComparator);

return documentMutationsStore(transaction)
.iterate({ range: keyRange }, (indexKey, _, control) => {
const [userID, encodedPath, batchID] = indexKey;
const path = EncodedResourcePath.decode(encodedPath);
if (
userID !== this.userId ||
Path.comparator(path, documentKeys.last().path) > 0
) {
control.done();
return;
}

if (!documentKeys.has(new DocumentKey(path))) {
return;
}
uniqueBatchIDs = uniqueBatchIDs.add(batchID);
})
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this is O(mutation queue size) instead of O(documentKeys size)... but can probably be fixed to be O(documentKeys size) by either calling iterate again for each documentKey, or by using control.skipToKey() after finding / not-finding each documentKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL.

.next(() => this.lookupMutationBatches(transaction, uniqueBatchIDs));
}

getAllMutationBatchesAffectingQuery(
transaction: PersistenceTransaction,
query: Query
Expand Down Expand Up @@ -413,29 +449,34 @@ export class IndexedDbMutationQueue implements MutationQueue {
}
uniqueBatchIDs = uniqueBatchIDs.add(batchID);
})
.next(() => {
const results: MutationBatch[] = [];
const promises: Array<PersistencePromise<void>> = [];
// TODO(rockwood): Implement this using iterate.
uniqueBatchIDs.forEach(batchID => {
const mutationKey = this.keyForBatchId(batchID);
promises.push(
mutationsStore(transaction)
.get(mutationKey)
.next(mutation => {
if (mutation === null) {
fail(
'Dangling document-mutation reference found, ' +
'which points to ' +
mutationKey
);
}
results.push(this.serializer.fromDbMutationBatch(mutation!));
})
);
});
return PersistencePromise.waitFor(promises).next(() => results);
});
.next(() => this.lookupMutationBatches(transaction, uniqueBatchIDs));
}

private lookupMutationBatches(
transaction: PersistenceTransaction,
batchIDs: SortedSet<BatchId>
): PersistencePromise<MutationBatch[]> {
const results: MutationBatch[] = [];
const promises: Array<PersistencePromise<void>> = [];
// TODO(rockwood): Implement this using iterate.
batchIDs.forEach(batchID => {
const mutationKey = this.keyForBatchId(batchID);
promises.push(
mutationsStore(transaction)
.get(mutationKey)
.next(mutation => {
if (mutation === null) {
fail(
'Dangling document-mutation reference found, ' +
'which points to ' +
mutationKey
);
}
results.push(this.serializer.fromDbMutationBatch(mutation!));
})
);
});
return PersistencePromise.waitFor(promises).next(() => results);
}

removeMutationBatches(
Expand Down
160 changes: 60 additions & 100 deletions packages/firestore/src/local/local_documents_view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import { Query } from '../core/query';
import { SnapshotVersion } from '../core/snapshot_version';
import {
documentKeySet,
DocumentKeySet,
DocumentMap,
documentMap,
Expand All @@ -26,6 +25,7 @@ import {
} from '../model/collections';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { MutationBatch } from '../model/mutation_batch';
import { ResourcePath } from '../model/path';
import { fail } from '../util/assert';

Expand Down Expand Up @@ -56,11 +56,23 @@ export class LocalDocumentsView {
transaction: PersistenceTransaction,
key: DocumentKey
): PersistencePromise<MaybeDocument | null> {
return this.remoteDocumentCache
.getEntry(transaction, key)
.next(remoteDoc => {
return this.computeLocalDocument(transaction, key, remoteDoc);
});
return this.mutationQueue
.getAllMutationBatchesAffectingDocumentKey(transaction, key)
.next(batches => this.getDocumentInBatches(transaction, key, batches));
}

/** Internal version of `getDocument` that allows reusing batches. */
private getDocumentInBatches(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not wild about getDocumentInBatches(). I'd probably rename to getDocumentInternal(). Or you could just combine with getDocument() and have inBatches be an optional param:

   getDocument(
    transaction: PersistenceTransaction,
    key: DocumentKey,
    inBatches?: MutationBatch[]) {
  let inBatchesPromise = inBatches ? PersistencePromise.resolve(inBatches) :
    this.mutationQueue.getAllMutationBatchesAffectingDocumentKey(transaction, key);

  return inBatchesPromise.next(inBatches =>
      this.remoteDocumentCache.getEntry(transaction, key))
    .next(doc => {
      for (const batch of inBatches) {
        doc = batch.applyToLocalView(key, doc);
      }
      return doc;
    });
}

(written via copy/paste and untested so probably not quite right)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any preference in favor of the optional param? I'm fine with that, but it would be a little less consistent with other platforms, where the other function is private. OTOH, perhaps it's more idiomatic?

Copy link
Contributor

Choose a reason for hiding this comment

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

No preference. You're right that it would ideally be private... but TypeScript doesn't support "overloads" with mixed visibility. So I think getDocumentInternal() is good.

transaction: PersistenceTransaction,
key: DocumentKey,
inBatches: MutationBatch[]
): PersistencePromise<MaybeDocument | null> {
return this.remoteDocumentCache.getEntry(transaction, key).next(doc => {
for (const batch of inBatches) {
doc = batch.applyToLocalView(key, doc);
}
return doc;
});
}

/**
Expand All @@ -73,20 +85,29 @@ export class LocalDocumentsView {
transaction: PersistenceTransaction,
keys: DocumentKeySet
): PersistencePromise<MaybeDocumentMap> {
const promises = [] as Array<PersistencePromise<void>>;
let results = maybeDocumentMap();
keys.forEach(key => {
promises.push(
this.getDocument(transaction, key).next(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 this.mutationQueue
.getAllMutationBatchesAffectingDocumentKeys(transaction, keys)
.next(batches => {
const promises = [] as Array<PersistencePromise<void>>;
let results = maybeDocumentMap();
keys.forEach(key => {
promises.push(
this.getDocumentInBatches(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);
}
)
);
});
return PersistencePromise.waitFor(promises).next(() => results);
});
}

/** Performs a query against the local view of all documents. */
Expand Down Expand Up @@ -122,48 +143,40 @@ export class LocalDocumentsView {
query: Query
): PersistencePromise<DocumentMap> {
// Query the remote documents and overlay mutations.
// TODO(mikelehen): There may be significant overlap between the mutations
// affecting these remote documents and the
// getAllMutationBatchesAffectingQuery() mutations. Consider optimizing.
let results: DocumentMap;
return this.remoteDocumentCache
.getDocumentsMatchingQuery(transaction, query)
.next(queryResults => {
return this.computeLocalDocuments(transaction, queryResults);
})
.next(promisedResults => {
results = promisedResults;
// Now use the mutation queue to discover any other documents that may
// match the query after applying mutations.
results = queryResults;
return this.mutationQueue.getAllMutationBatchesAffectingQuery(
transaction,
query
);
})
.next(matchingMutationBatches => {
let matchingKeys = documentKeySet();
for (const batch of matchingMutationBatches) {
for (const mutation of batch.mutations) {
// TODO(mikelehen): PERF: Check if this mutation actually
// affects the query to reduce work.
if (!results.get(mutation.key)) {
matchingKeys = matchingKeys.add(mutation.key);
const key = mutation.key;
// Only process documents belonging to the collection.
if (!query.path.isImmediateParentOf(key.path)) {
continue;
}

const baseDoc = results.get(key);
const mutatedDoc = mutation.applyToLocalView(
baseDoc,
baseDoc,
batch.localWriteTime
);
if (!mutatedDoc || mutatedDoc instanceof NoDocument) {
results = results.remove(key);
} else if (mutatedDoc instanceof Document) {
results = results.insert(key, mutatedDoc);
} else {
fail('Unknown MaybeDocument: ' + mutatedDoc);
}
}
}

// Now add in the results for the matchingKeys.
const promises = [] as Array<PersistencePromise<void>>;
matchingKeys.forEach(key => {
promises.push(
this.getDocument(transaction, key).next(doc => {
if (doc instanceof Document) {
results = results.insert(doc.key, doc);
}
})
);
});
return PersistencePromise.waitFor(promises);
})
.next(() => {
// Finally, filter out any documents that don't actually match
Expand All @@ -177,57 +190,4 @@ export class LocalDocumentsView {
return results;
});
}

/**
* Takes a remote document and applies local mutations to generate the local
* view of the document.
* @param transaction The transaction in which to perform any persistence
* operations.
* @param documentKey The key of the document (necessary when remoteDocument
* is null).
* @param document The base remote document to apply mutations to or null.
*/
private computeLocalDocument(
transaction: PersistenceTransaction,
documentKey: DocumentKey,
document: MaybeDocument | null
): PersistencePromise<MaybeDocument | null> {
return this.mutationQueue
.getAllMutationBatchesAffectingDocumentKey(transaction, documentKey)
.next(batches => {
for (const batch of batches) {
document = batch.applyToLocalView(documentKey, document);
}
return document;
});
}

/**
* Takes a set of remote documents and applies local mutations to generate the
* local view of the documents.
* @param transaction The transaction in which to perform any persistence
* operations.
* @param documents The base remote documents to apply mutations to.
* @return The local view of the documents.
*/
private computeLocalDocuments(
transaction: PersistenceTransaction,
documents: DocumentMap
): PersistencePromise<DocumentMap> {
const promises = [] as Array<PersistencePromise<void>>;
documents.forEach((key, doc) => {
promises.push(
this.computeLocalDocument(transaction, key, doc).next(mutatedDoc => {
if (mutatedDoc instanceof Document) {
documents = documents.insert(mutatedDoc.key, mutatedDoc);
} else if (mutatedDoc instanceof NoDocument) {
documents = documents.remove(mutatedDoc.key);
} else {
fail('Unknown MaybeDocument: ' + mutatedDoc);
}
})
);
});
return PersistencePromise.waitFor(promises).next(() => documents);
}
}
29 changes: 27 additions & 2 deletions packages/firestore/src/local/memory_mutation_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import { Timestamp } from '../api/timestamp';
import { Query } from '../core/query';
import { BatchId, ProtoByteString } from '../core/types';
import { DocumentKeySet } from '../model/collections';
import { DocumentKey } from '../model/document_key';
import { Mutation } from '../model/mutation';
import { BATCHID_UNKNOWN, MutationBatch } from '../model/mutation_batch';
Expand Down Expand Up @@ -252,6 +253,26 @@ export class MemoryMutationQueue implements MutationQueue {
return PersistencePromise.resolve(result);
}

getAllMutationBatchesAffectingDocumentKeys(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
): PersistencePromise<MutationBatch[]> {
if (documentKeys.isEmpty()) {
return PersistencePromise.resolve([]);
}
const start = new DocReference(documentKeys.first(), 0);
const end = new DocReference(documentKeys.last(), Number.POSITIVE_INFINITY);
let uniqueBatchIDs = new SortedSet<number>(primitiveComparator);

this.batchesByDocumentKey.forEachInRange([start, end], ref => {
if (documentKeys.has(ref.key)) {
uniqueBatchIDs = uniqueBatchIDs.add(ref.targetOrBatchId);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is O(mutation queue) instead of O(document keys). Can we make it match iOS / Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL.


return PersistencePromise.resolve(this.findMutationBatches(uniqueBatchIDs));
}

getAllMutationBatchesAffectingQuery(
transaction: PersistenceTransaction,
query: Query
Expand Down Expand Up @@ -293,16 +314,20 @@ export class MemoryMutationQueue implements MutationQueue {
}
}, start);

return PersistencePromise.resolve(this.findMutationBatches(uniqueBatchIDs));
}

private findMutationBatches(batchIDs: SortedSet<number>): MutationBatch[] {
// Construct an array of matching batches, sorted by batchID to ensure that
// multiple mutations affecting the same document key are applied in order.
const result: MutationBatch[] = [];
uniqueBatchIDs.forEach(batchId => {
batchIDs.forEach(batchId => {
const batch = this.findMutationBatch(batchId);
if (batch !== null) {
result.push(batch);
}
});
return PersistencePromise.resolve(result);
return result;
}

removeMutationBatches(
Expand Down
Loading