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 7 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
46 changes: 26 additions & 20 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 @@ -22,7 +22,7 @@ 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 { Path, ResourcePath } from '../model/path';
import { 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 @@ -367,34 +367,40 @@ export class IndexedDbMutationQueue implements MutationQueue {
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 promises: Array<PersistencePromise<void>> = [];
documentKeys.forEach(documentKey => {
const indexStart = DbDocumentMutation.prefixForPath(
this.userId,
documentKey.path
);
const range = IDBKeyRange.lowerBound(indexStart);

const promise = documentMutationsStore(transaction)
.iterate({ range }, (indexKey, _, control) => {
const [userID, encodedPath, batchID] = indexKey;

// Only consider rows matching exactly the specific key of
// interest. Note that because we order by path first, and we
// order terminators before path separators, we'll encounter all
// the index rows for documentKey contiguously. In particular, all
// the rows for documentKey will occur before any rows for
// documents nested in a subcollection beneath documentKey so we
// can stop as soon as we hit any such row.
const path = EncodedResourcePath.decode(encodedPath);
if (
userID !== this.userId ||
Path.comparator(path, documentKeys.last().path) > 0
) {
if (userID !== this.userId || !documentKey.path.isEqual(path)) {
control.done();
return;
}

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

promises.push(promise);
});

return PersistencePromise.waitFor(promises)
.next(() => this.lookupMutationBatches(transaction, uniqueBatchIDs));
}

Expand Down
6 changes: 3 additions & 3 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 @@ -58,11 +58,11 @@ export class LocalDocumentsView {
): PersistencePromise<MaybeDocument | null> {
return this.mutationQueue
.getAllMutationBatchesAffectingDocumentKey(transaction, key)
.next(batches => this.getDocumentInBatches(transaction, key, batches));
.next(batches => this.getDocumentInternal(transaction, key, batches));
}

/** Internal version of `getDocument` that allows reusing batches. */
private getDocumentInBatches(
private getDocumentInternal(
transaction: PersistenceTransaction,
key: DocumentKey,
inBatches: MutationBatch[]
Expand Down Expand Up @@ -92,7 +92,7 @@ export class LocalDocumentsView {
let results = maybeDocumentMap();
keys.forEach(key => {
promises.push(
this.getDocumentInBatches(transaction, key, batches).next(
this.getDocumentInternal(transaction, key, batches).next(
maybeDoc => {
// TODO(http://b/32275378): Don't conflate missing / deleted.
if (!maybeDoc) {
Expand Down
17 changes: 9 additions & 8 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 @@ -257,17 +257,18 @@ export class MemoryMutationQueue implements MutationQueue {
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)) {
documentKeys.forEach(documentKey => {
const start = new DocReference(documentKey, 0);
const end = new DocReference(documentKey, Number.POSITIVE_INFINITY);
this.batchesByDocumentKey.forEachInRange([start, end], ref => {
if (!documentKey.isEqual(ref.key)) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is it possible to hit this? Seems like our range should constrain us to 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.

I'm not sure. This function is derived from getAllMutationBatchesAffectingDocumentKey, which asserts this equality, this led me to believe this case is not impossible. I can remove it altogether or replace with an assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Asserts should generally mean the case is impossible. They exist to document our expectations and/or catch false expectations as early as possible to make finding / fixing bugs later. So I think an assert here makes perfect sense.

Fine print: We're probably not 100% rigorous about asserts always being impossible. E.g. there may be cases where we use asserts for cases that /are/ possible but represent rare error conditions that we don't feel compelled to provide proper error handling for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replicated the assertion.

}

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));
Expand Down
2 changes: 0 additions & 2 deletions packages/firestore/src/local/mutation_queue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ export interface MutationQueue extends GarbageSource {
* convenient.
*/
// TODO(mcg): This should really return an enumerator
// also for b/32992024, all backing stores should really index by document key
getAllMutationBatchesAffectingDocumentKey(
transaction: PersistenceTransaction,
documentKey: DocumentKey
Expand All @@ -166,7 +165,6 @@ export interface MutationQueue extends GarbageSource {
* convenient.
*/
// TODO(mcg): This should really return an enumerator
// also for b/32992024, all backing stores should really index by document key
getAllMutationBatchesAffectingDocumentKeys(
transaction: PersistenceTransaction,
documentKeys: DocumentKeySet
Expand Down