Implement Eager Reference Delegate for Memory Persistence, drop old GC (#1256)

* Implement eager reference delegate for memory

* Removing old garbage collector and fixing tests

* Remove sequential forEach, replace with parallel forEach

Author: Greg Soltis

packages/firestore/src/core/firestore_client.ts

@@ -16,12 +16,9 @@
 
 import { CredentialsProvider } from '../api/credentials';
 import { User } from '../auth/user';
-import { EagerGarbageCollector } from '../local/eager_garbage_collector';
-import { GarbageCollector } from '../local/garbage_collector';
 import { IndexedDbPersistence } from '../local/indexeddb_persistence';
 import { LocalStore } from '../local/local_store';
 import { MemoryPersistence } from '../local/memory_persistence';
-import { NoOpGarbageCollector } from '../local/no_op_garbage_collector';
 import { Persistence } from '../local/persistence';
 import {
   DocumentKeySet,
@@ -83,7 +80,6 @@ export class FirestoreClient {
   // with the types rather than littering the code with '!' or unnecessary
   // undefined checks.
   private eventMgr: EventManager;
-  private garbageCollector: GarbageCollector;
   private persistence: Persistence;
   private localStore: LocalStore;
   private remoteStore: RemoteStore;
@@ -292,7 +288,6 @@ export class FirestoreClient {
 
     // TODO(http://b/33384523): For now we just disable garbage collection
     // when persistence is enabled.
-    this.garbageCollector = new NoOpGarbageCollector();
     const storagePrefix = IndexedDbPersistence.buildStoragePrefix(
       this.databaseInfo
     );
@@ -347,8 +342,7 @@ export class FirestoreClient {
    * @returns A promise that will successfully resolve.
    */
   private startMemoryPersistence(): Promise<void> {
-    this.garbageCollector = new EagerGarbageCollector();
-    this.persistence = new MemoryPersistence(this.clientId);
+    this.persistence = MemoryPersistence.createEagerPersistence(this.clientId);
     this.sharedClientState = new MemorySharedClientState();
     return Promise.resolve();
   }
@@ -363,11 +357,7 @@ export class FirestoreClient {
     return this.platform
       .loadConnection(this.databaseInfo)
       .then(async connection => {
-        this.localStore = new LocalStore(
-          this.persistence,
-          user,
-          this.garbageCollector
-        );
+        this.localStore = new LocalStore(this.persistence, user);
         const serializer = this.platform.newSerializer(
           this.databaseInfo.databaseId
         );

packages/firestore/src/core/sync_engine.ts

@@ -324,7 +324,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
             this.remoteStore.unlisten(queryView.targetId);
             return this.removeAndCleanupQuery(queryView);
           })
-          .then(() => this.localStore.collectGarbage())
           .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
       }
     } else {
@@ -583,7 +582,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       // NOTE: Both these methods are no-ops for batches that originated from
       // other clients.
       this.processUserCallback(batchId, error ? error : null);
-
       this.localStore.removeCachedMutationBatchMetadata(batchId);
     } else {
       fail(`Unknown batchState: ${batchState}`);
@@ -820,12 +818,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
     await Promise.all(queriesProcessed);
     this.syncEngineListener!.onWatchChange(newSnaps);
-    this.localStore.notifyLocalViewChanges(docChangesInAllViews);
-    if (this.isPrimary) {
-      await this.localStore
-        .collectGarbage()
-        .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
-    }
+    await this.localStore.notifyLocalViewChanges(docChangesInAllViews);
   }
 
   /**

packages/firestore/src/local/eager_garbage_collector.ts

@@ -28,7 +28,6 @@ import { primitiveComparator } from '../util/misc';
 import { SortedSet } from '../util/sorted_set';
 
 import * as EncodedResourcePath from './encoded_resource_path';
-import { GarbageCollector } from './garbage_collector';
 import {
   IndexedDbPersistence,
   IndexedDbTransaction
@@ -43,7 +42,7 @@ import {
 } from './indexeddb_schema';
 import { LocalSerializer } from './local_serializer';
 import { MutationQueue } from './mutation_queue';
-import { PersistenceTransaction } from './persistence';
+import { PersistenceTransaction, ReferenceDelegate } from './persistence';
 import { PersistencePromise } from './persistence_promise';
 import { SimpleDbStore, SimpleDbTransaction } from './simple_db';
 
@@ -63,15 +62,14 @@ export class IndexedDbMutationQueue implements MutationQueue {
   // PORTING NOTE: Multi-tab only.
   private documentKeysByBatchId = {} as { [batchId: number]: DocumentKeySet };
 
-  private garbageCollector: GarbageCollector | null = null;
-
   constructor(
     /**
      * The normalized userId (e.g. null UID => "" userId) used to store /
      * retrieve mutations.
      */
     private userId: string,
-    private serializer: LocalSerializer
+    private serializer: LocalSerializer,
+    private readonly referenceDelegate: ReferenceDelegate
   ) {}
 
   /**
@@ -81,15 +79,16 @@ export class IndexedDbMutationQueue implements MutationQueue {
    */
   static forUser(
     user: User,
-    serializer: LocalSerializer
+    serializer: LocalSerializer,
+    referenceDelegate: ReferenceDelegate
   ): IndexedDbMutationQueue {
     // TODO(mcg): Figure out what constraints there are on userIDs
     // In particular, are there any reserved characters? are empty ids allowed?
     // For the moment store these together in the same mutations table assuming
     // that empty userIDs aren't allowed.
     assert(user.uid !== '', 'UserID must not be an empty string.');
     const userId = user.isAuthenticated() ? user.uid! : '';
-    return new IndexedDbMutationQueue(userId, serializer);
+    return new IndexedDbMutationQueue(userId, serializer, referenceDelegate);
   }
 
   start(transaction: PersistenceTransaction): PersistencePromise<void> {
@@ -470,12 +469,9 @@ export class IndexedDbMutationQueue implements MutationQueue {
       batch
     ).next(removedDocuments => {
       this.removeCachedMutationKeys(batch.batchId);
-      if (this.garbageCollector !== null) {
-        for (const key of removedDocuments) {
-          // TODO(gsoltis): tell reference delegate that mutation was ack'd
-          this.garbageCollector.addPotentialGarbageKey(key);
-        }
-      }
+      return PersistencePromise.forEach(removedDocuments, key => {
+        return this.referenceDelegate.removeMutationReference(transaction, key);
+      });
     });
   }
 
@@ -519,10 +515,6 @@ export class IndexedDbMutationQueue implements MutationQueue {
     });
   }
 
-  setGarbageCollector(gc: GarbageCollector | null): void {
-    this.garbageCollector = gc;
-  }
-
   containsKey(
     txn: PersistenceTransaction,
     key: DocumentKey