-
Notifications
You must be signed in to change notification settings - Fork 938
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
Changes from all commits
3da020e
9717fef
204443e
c56820b
f3174ca
36c53d3
942b0fa
7767b60
a1ad6d2
cc29731
307e0b3
62c627b
8f293eb
4e23f3b
fb751dd
431f618
1681f3d
55c7ff3
5afa305
56eefbc
5e506fa
7b11cec
1a32b22
17a69a6
4c270c7
512667e
6cb4bfb
77bf92e
e7b8c8e
a0d25f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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( | ||
transaction: PersistenceTransaction, | ||
documentKeys: DocumentKeySet | ||
): PersistencePromise<DocumentSizeEntries> { | ||
let results = nullableMaybeDocumentMap(); | ||
let sizeMap = new SortedMap<DocumentKey, number>(DocumentKey.comparator); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It's probably not a big deal, so if you think code clarity is more important here, I'll do the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 { | ||
|
There was a problem hiding this comment.
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 aSortedMap<DocumentKey, DocumentSizeEntry>
. I decided in favor of returning two maps because it makes it easier to avoid code duplication betweengetEntries
andgetSizedEntries
.There was a problem hiding this comment.
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 bePersistencePromise<SortedMap<DocumentKey, T>>
. You can avoid code duplication and avoid doing extra work for sizes that way.There was a problem hiding this comment.
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
inRemoteDocumentChangeBuffer
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 afn
that either updates thesizeMap
or is a no-op).There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.