From d6126e9009f9b787d039069bf46810798eb29ce7 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 30 Jan 2023 11:14:47 -0500 Subject: [PATCH 1/3] Rename --- .../firestore/src/local/indexeddb_remote_document_cache.ts | 2 +- packages/firestore/src/local/local_documents_view.ts | 2 +- packages/firestore/src/local/memory_remote_document_cache.ts | 2 +- packages/firestore/src/local/remote_document_cache.ts | 2 +- packages/firestore/test/unit/local/counting_query_engine.ts | 4 ++-- .../firestore/test/unit/local/test_remote_document_cache.ts | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 19b7c566322..72207207be4 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -273,7 +273,7 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { }); } - getAllFromCollection( + getDocumentsMatchingQuery( transaction: PersistenceTransaction, collection: ResourcePath, offset: IndexOffset diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 2572646655f..16a6931831f 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -511,7 +511,7 @@ export class LocalDocumentsView { // Query the remote documents and overlay mutations. let remoteDocuments: MutableDocumentMap; return this.remoteDocumentCache - .getAllFromCollection(transaction, query.path, offset) + .getDocumentsMatchingQuery(transaction, query.path, offset) .next(queryResults => { remoteDocuments = queryResults; return this.documentOverlayCache.getOverlaysForCollection( diff --git a/packages/firestore/src/local/memory_remote_document_cache.ts b/packages/firestore/src/local/memory_remote_document_cache.ts index 796e461295d..c29ff1413a1 100644 --- a/packages/firestore/src/local/memory_remote_document_cache.ts +++ b/packages/firestore/src/local/memory_remote_document_cache.ts @@ -159,7 +159,7 @@ class MemoryRemoteDocumentCacheImpl implements MemoryRemoteDocumentCache { return PersistencePromise.resolve(results); } - getAllFromCollection( + getDocumentsMatchingQuery( transaction: PersistenceTransaction, collectionPath: ResourcePath, offset: IndexOffset diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index 55c15826d3a..215d4f095da 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -68,7 +68,7 @@ export interface RemoteDocumentCache { * @param offset - The offset to start the scan at (exclusive). * @returns The set of matching documents. */ - getAllFromCollection( + getDocumentsMatchingQuery( transaction: PersistenceTransaction, collection: ResourcePath, offset: IndexOffset diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index a8a89a590b6..8fc655c443d 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -101,9 +101,9 @@ export class CountingQueryEngine extends QueryEngine { setIndexManager: (indexManager: IndexManager) => { subject.setIndexManager(indexManager); }, - getAllFromCollection: (transaction, collection, sinceReadTime) => { + getDocumentsMatchingQuery: (transaction, collection, sinceReadTime) => { return subject - .getAllFromCollection(transaction, collection, sinceReadTime) + .getDocumentsMatchingQuery(transaction, collection, sinceReadTime) .next(result => { this.documentsReadByCollection += result.size; return result; diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 62344dd4b41..429b03a5e4e 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/packages/firestore/test/unit/local/test_remote_document_cache.ts @@ -116,7 +116,7 @@ export class TestRemoteDocumentCache { return this.persistence.runTransaction( 'getAllFromCollection', 'readonly', - txn => this.cache.getAllFromCollection(txn, collection, offset) + txn => this.cache.getDocumentsMatchingQuery(txn, collection, offset) ); } From 538202809cb66076ac627f97cc30c7fe8bc50a77 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 30 Jan 2023 12:06:08 -0500 Subject: [PATCH 2/3] First commit --- .../local/indexeddb_remote_document_cache.ts | 18 +++- .../src/local/local_documents_view.ts | 19 +++-- .../src/local/memory_remote_document_cache.ts | 16 +++- .../src/local/remote_document_cache.ts | 17 ++-- .../test/unit/local/counting_query_engine.ts | 14 +++- .../test/unit/local/local_store.test.ts | 4 +- .../unit/local/remote_document_cache.test.ts | 84 +++++++++++++++---- .../unit/local/test_remote_document_cache.ts | 15 ++-- 8 files changed, 139 insertions(+), 48 deletions(-) diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 72207207be4..c3af0655cc4 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -15,12 +15,14 @@ * limitations under the License. */ +import { Query, queryMatches } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, DocumentSizeEntries, MutableDocumentMap, - mutableDocumentMap + mutableDocumentMap, + OverlayMap } from '../model/collections'; import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -275,9 +277,11 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { getDocumentsMatchingQuery( transaction: PersistenceTransaction, - collection: ResourcePath, - offset: IndexOffset + query: Query, + offset: IndexOffset, + mutatedDocs: OverlayMap ): PersistencePromise { + const collection = query.path; const startKey = [ collection.popLast().toArray(), collection.lastSegment(), @@ -307,7 +311,13 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { ), dbRemoteDoc ); - results = results.insert(document.key, document); + if ( + document.isFoundDocument() && + (queryMatches(query, document) || mutatedDocs.has(document.key)) + ) { + // Either the document matches the given query, or it is mutated. + results = results.insert(document.key, document); + } } return results; }); diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 16a6931831f..78802e443bf 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -509,18 +509,19 @@ export class LocalDocumentsView { offset: IndexOffset ): PersistencePromise { // Query the remote documents and overlay mutations. - let remoteDocuments: MutableDocumentMap; - return this.remoteDocumentCache - .getDocumentsMatchingQuery(transaction, query.path, offset) - .next(queryResults => { - remoteDocuments = queryResults; - return this.documentOverlayCache.getOverlaysForCollection( + let overlays: OverlayMap; + return this.documentOverlayCache + .getOverlaysForCollection(transaction, query.path, offset.largestBatchId) + .next(result => { + overlays = result; + return this.remoteDocumentCache.getDocumentsMatchingQuery( transaction, - query.path, - offset.largestBatchId + query, + offset, + overlays ); }) - .next(overlays => { + .next(remoteDocuments => { // As documents might match the query because of their overlay we need to // include documents for all overlays in the initial document set. overlays.forEach((_, overlay) => { diff --git a/packages/firestore/src/local/memory_remote_document_cache.ts b/packages/firestore/src/local/memory_remote_document_cache.ts index c29ff1413a1..1f598ba6cf9 100644 --- a/packages/firestore/src/local/memory_remote_document_cache.ts +++ b/packages/firestore/src/local/memory_remote_document_cache.ts @@ -15,11 +15,13 @@ * limitations under the License. */ +import { Query, queryMatches } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, MutableDocumentMap, - mutableDocumentMap + mutableDocumentMap, + OverlayMap } from '../model/collections'; import { Document, MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -28,7 +30,6 @@ import { indexOffsetComparator, newIndexOffsetFromDocument } from '../model/field_index'; -import { ResourcePath } from '../model/path'; import { debugAssert, fail } from '../util/assert'; import { SortedMap } from '../util/sorted_map'; @@ -161,13 +162,15 @@ class MemoryRemoteDocumentCacheImpl implements MemoryRemoteDocumentCache { getDocumentsMatchingQuery( transaction: PersistenceTransaction, - collectionPath: ResourcePath, - offset: IndexOffset + query: Query, + offset: IndexOffset, + mutatedDocs: OverlayMap ): PersistencePromise { let results = mutableDocumentMap(); // Documents are ordered by key, so we can use a prefix scan to narrow down // the documents we need to match the query against. + const collectionPath = query.path; const prefix = new DocumentKey(collectionPath.child('')); const iterator = this.docs.getIteratorFrom(prefix); while (iterator.hasNext()) { @@ -188,6 +191,11 @@ class MemoryRemoteDocumentCacheImpl implements MemoryRemoteDocumentCache { // The document sorts before the offset. continue; } + if (!mutatedDocs.has(document.key) && !queryMatches(query, document)) { + // The document cannot possibly match the query. + continue; + } + results = results.insert(document.key, document.mutableCopy()); } return PersistencePromise.resolve(results); diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index 215d4f095da..a66f1b4a253 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -15,11 +15,15 @@ * limitations under the License. */ -import { DocumentKeySet, MutableDocumentMap } from '../model/collections'; +import { Query } from '../core/query'; +import { + DocumentKeySet, + MutableDocumentMap, + OverlayMap +} from '../model/collections'; import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { IndexOffset } from '../model/field_index'; -import { ResourcePath } from '../model/path'; import { IndexManager } from './index_manager'; import { PersistencePromise } from './persistence_promise'; @@ -62,16 +66,17 @@ export interface RemoteDocumentCache { ): PersistencePromise; /** - * Returns the documents from the provided collection. + * Returns the documents matching the given query * - * @param collection - The collection to read. + * @param query - The query to match documents against. * @param offset - The offset to start the scan at (exclusive). * @returns The set of matching documents. */ getDocumentsMatchingQuery( transaction: PersistenceTransaction, - collection: ResourcePath, - offset: IndexOffset + query: Query, + offset: IndexOffset, + mutatedDocs: OverlayMap ): PersistencePromise; /** diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index 8fc655c443d..d407abfd60a 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -101,9 +101,19 @@ export class CountingQueryEngine extends QueryEngine { setIndexManager: (indexManager: IndexManager) => { subject.setIndexManager(indexManager); }, - getDocumentsMatchingQuery: (transaction, collection, sinceReadTime) => { + getDocumentsMatchingQuery: ( + transaction, + query, + sinceReadTime, + overlays + ) => { return subject - .getDocumentsMatchingQuery(transaction, collection, sinceReadTime) + .getDocumentsMatchingQuery( + transaction, + query, + sinceReadTime, + overlays + ) .next(result => { this.documentsReadByCollection += result.size; return result; diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index c25d337dc13..204f714a486 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -2288,10 +2288,10 @@ function genericLocalStoreTests( .afterAcknowledgingMutation({ documentVersion: 10 }) .afterAcknowledgingMutation({ documentVersion: 10 }) .afterExecutingQuery(query1) - // Execute the query, but note that we read all existing documents + // Execute the query, but note that we read matching documents // from the RemoteDocumentCache since we do not yet have target // mapping. - .toHaveRead({ documentsByCollection: 3 }) + .toHaveRead({ documentsByCollection: 2 }) .after( docAddedRemoteEvent( [ diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index 0bed7c4dade..7090394a9c8 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -19,7 +19,11 @@ import { expect } from 'chai'; import { User } from '../../../src/auth/user'; import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence'; -import { documentKeySet, DocumentMap } from '../../../src/model/collections'; +import { + documentKeySet, + DocumentMap, + newOverlayMap +} from '../../../src/model/collections'; import { MutableDocument, Document } from '../../../src/model/document'; import { IndexOffset, @@ -27,13 +31,15 @@ import { newIndexOffsetFromDocument, newIndexOffsetSuccessorFromReadTime } from '../../../src/model/field_index'; +import { Overlay } from '../../../src/model/overlay'; import { deletedDoc, doc, expectEqual, field, + filter, key, - path, + query, version, wrap } from '../../util/helpers'; @@ -464,9 +470,10 @@ function genericRemoteDocumentCacheTests( doc('c/1', VERSION, DOC_DATA) ]); - const matchingDocs = await cache.getAllFromCollection( - path('b'), - IndexOffset.min() + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('b'), + IndexOffset.min(), + newOverlayMap() ); assertMatches( [doc('b/1', VERSION, DOC_DATA), doc('b/2', VERSION, DOC_DATA)], @@ -480,9 +487,10 @@ function genericRemoteDocumentCacheTests( doc('a/1/b/1', VERSION, DOC_DATA) ]); - const matchingDocs = await cache.getAllFromCollection( - path('a'), - IndexOffset.min() + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('a'), + IndexOffset.min(), + newOverlayMap() ); assertMatches([doc('a/1', VERSION, DOC_DATA)], matchingDocs); }); @@ -498,20 +506,62 @@ function genericRemoteDocumentCacheTests( doc('b/new', 3, DOC_DATA).setReadTime(version(13)) ]); - const matchingDocs = await cache.getAllFromCollection( - path('b'), - newIndexOffsetSuccessorFromReadTime(version(12), INITIAL_LARGEST_BATCH_ID) + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('b'), + newIndexOffsetSuccessorFromReadTime( + version(12), + INITIAL_LARGEST_BATCH_ID + ), + newOverlayMap() ); assertMatches([doc('b/new', 3, DOC_DATA)], matchingDocs); }); + it('getDocumentsMatchingQuery() applies query check', async () => { + await cache.addEntries([ + doc('a/1', 1, { matches: true }).setReadTime(version(1)) + ]); + await cache.addEntries([ + doc('a/2', 1, { matches: true }).setReadTime(version(2)) + ]); + await cache.addEntries([ + doc('a/3', 1, { matches: false }).setReadTime(version(3)) + ]); + + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('a', filter('matches', '==', true)), + newIndexOffsetSuccessorFromReadTime(version(1), INITIAL_LARGEST_BATCH_ID), + newOverlayMap() + ); + assertMatches([doc('a/2', 1, { matches: true })], matchingDocs); + }); + + it('getDocumentsMatchingQuery() respects mutated documents', async () => { + await cache.addEntries([ + doc('a/1', 1, { matches: true }).setReadTime(version(1)) + ]); + await cache.addEntries([ + doc('a/2', 1, { matches: false }).setReadTime(version(2)) + ]); + + const mutatedDocs = newOverlayMap(); + mutatedDocs.set(key('a/2'), {} as Overlay); + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('a', filter('matches', '==', true)), + newIndexOffsetSuccessorFromReadTime(version(1), INITIAL_LARGEST_BATCH_ID), + mutatedDocs + ); + assertMatches([doc('a/2', 1, { matches: false })], matchingDocs); + }); + it('getAll() uses read time rather than update time', async () => { await cache.addEntries([doc('b/old', 1, DOC_DATA).setReadTime(version(2))]); await cache.addEntries([doc('b/new', 2, DOC_DATA).setReadTime(version(1))]); - const matchingDocs = await cache.getAllFromCollection( - path('b'), - newIndexOffsetSuccessorFromReadTime(version(1), INITIAL_LARGEST_BATCH_ID) + const matchingDocs = await cache.getDocumentsMatchingQuery( + query('b'), + newIndexOffsetSuccessorFromReadTime(version(1), INITIAL_LARGEST_BATCH_ID), + newOverlayMap() ); assertMatches([doc('b/old', 1, DOC_DATA)], matchingDocs); }); @@ -545,7 +595,11 @@ function genericRemoteDocumentCacheTests( document.data.set(field('state'), wrap('new')); document = await cache - .getAllFromCollection(path('coll'), IndexOffset.min()) + .getDocumentsMatchingQuery( + query('coll'), + IndexOffset.min(), + newOverlayMap() + ) .then(m => m.get(key('coll/doc'))!); verifyOldValue(document); diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 429b03a5e4e..16037e4ea90 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/packages/firestore/test/unit/local/test_remote_document_cache.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import { Query } from '../../../src/core/query'; import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { IndexManager } from '../../../src/local/index_manager'; import { Persistence } from '../../../src/local/persistence'; @@ -23,12 +24,12 @@ import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; import { RemoteDocumentChangeBuffer } from '../../../src/local/remote_document_change_buffer'; import { DocumentKeySet, - MutableDocumentMap + MutableDocumentMap, + OverlayMap } from '../../../src/model/collections'; import { Document, MutableDocument } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; import { IndexOffset } from '../../../src/model/field_index'; -import { ResourcePath } from '../../../src/model/path'; /** * A wrapper around a RemoteDocumentCache that automatically creates a @@ -109,14 +110,16 @@ export class TestRemoteDocumentCache { }); } - getAllFromCollection( - collection: ResourcePath, - offset: IndexOffset + getDocumentsMatchingQuery( + query: Query, + offset: IndexOffset, + mutatedDocs: OverlayMap ): Promise { return this.persistence.runTransaction( 'getAllFromCollection', 'readonly', - txn => this.cache.getDocumentsMatchingQuery(txn, collection, offset) + txn => + this.cache.getDocumentsMatchingQuery(txn, query, offset, mutatedDocs) ); } From b7e43499e436c991a95e4f223ca2345a77f17e8e Mon Sep 17 00:00:00 2001 From: wu-hui <53845758+wu-hui@users.noreply.github.com> Date: Mon, 30 Jan 2023 19:39:56 -0500 Subject: [PATCH 3/3] Create slimy-elephants-hear.md --- .changeset/slimy-elephants-hear.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/slimy-elephants-hear.md diff --git a/.changeset/slimy-elephants-hear.md b/.changeset/slimy-elephants-hear.md new file mode 100644 index 00000000000..cd1d8d2df4e --- /dev/null +++ b/.changeset/slimy-elephants-hear.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Reduce memory usage by applying query check sooner in remote document cache.