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

gsoltis · 2018-09-26T22:46:11Z

In this PR:

Implement MemoryEagerDelegate, implements eager gc for memory persistence
Remove GarbageCollector, GarbageSource and implementations / extensions
Fix up tests, mostly in local store.

Final portion of LRU port to webclient
Android Port: FirebasePrivate/firebase-android-sdk#190
iOS Port: firebase/firebase-ios-sdk#1600

schmidt-sebastian

This is pretty much good. Some suggestions, and I wonder if waitForAll could be simplified (see #1269)

schmidt-sebastian · 2018-10-01T18:21:25Z

packages/firestore/src/local/persistence_promise.ts

+   * each element in the collection and wait for all of the resulting
+   * concurrent PersistencePromises to resolve.
+   */
+  static waitForEach<K>(


I just revived one of my older PRs that would allow you to merge this with forEach above. Let me know if you think this could work for you: #1269

The two methods are still different. forEach runs the callbacks serially, while waitForEach runs all of the callbacks and then waits for all of them.

However, I rewrote waitForEach to take an Iterable, which I think is more generally useful.

I feel like my comment on this got lost somehow. If you do think that the parallelism improves things, can we make the difference between forEach/waitFor/waitForEach more explicit? We could maybe rename waitForEach to forEachParalellel or something similar (sorry for the horrible name suggestion) or pass in a flag that allows parallelization. For what it's worth, there are currently other calls to forEach that could be parallelized.

I am removing the old forEach and renaming waitForEach to forEach. Everything will be parallel.

schmidt-sebastian · 2018-10-01T18:25:28Z

packages/firestore/src/local/indexeddb_mutation_queue.ts

-        }
-      }
+      return PersistencePromise.waitForEach(removedDocuments, key => {
+        return this.referenceDelegate.removeMutationReference(transaction, key);


This line seems to be missing from Android (at least in master). Is that still coming?

Yes. I have a few changes that I need to make to Android as a result of porting. This is one of them.

schmidt-sebastian · 2018-10-01T18:28:43Z

packages/firestore/src/local/indexeddb_query_cache.ts

+      return PersistencePromise.waitFor([
+        store.delete([targetId, path]),
+        this.referenceDelegate.removeReference(txn, key)
+      ]);


This could just be:

return store.delete(..).next(() => this.referenceDelegate.removeReference(txn, key))

Unless you have data that the waitFor parallelism meaningfully improves performance :)

Shouldn't we default to concurrent? We're potentially building up a big list of operations here, is there a reason to sequence any of them?

There are only two promises here, and one of them is a delete for a single key.

Yes, but it's in a loop. We can have a bunch of concurrent promises, or half as many two-step promises. I'd prefer to default to concurrent, especially for things that hit disk.

packages/firestore/src/local/local_store.ts

packages/firestore/src/local/memory_persistence.ts

schmidt-sebastian · 2018-10-01T18:44:47Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -244,14 +235,28 @@ class LocalStoreTester {
    return this;
  }

+  toNotContainIfEager(


Nice! I would maybe flip this to toContainIfLru, since that feels like the stricter assert. But that's just my personal opinion :)

I actually think it's stronger this way. It's asserting that eager gc will remove the document. LRU won't, no-op won't, and potentially other GC algorithms wouldn't either.

schmidt-sebastian · 2018-10-01T18:45:30Z

packages/firestore/test/unit/local/local_store.test.ts

+    gcIsEager: boolean,
+    doc: MaybeDocument
+  ): LocalStoreTester {
+    if (gcIsEager) {


Can we pass gcIsEager to LocalStoreTester? That way, the tests would not need to know what GC mode they are running under.

Yes, and that simplifies .toNotContainIfEager() calls. But, I think some tests will still need to check? Unless those get moved to the appropriate subclass.

schmidt-sebastian · 2018-10-01T18:47:46Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -643,22 +639,25 @@ function genericLocalStoreTests(
  });

  it('handles SetMutation -> Ack -> PatchMutation -> Reject', () => {


This test could be moved to the LocalStore w/ Memory Persistence "subclass".

I started implementing this, but I think it gets a little too ugly with sharing the test infrastructure with the generic tests.

If you feel strongly we can refactor, but I'd like to do it in a separate PR.

I was hoping that you could just move the tests method to the respective class, similar to what we do here:

firebase-js-sdk/packages/firestore/test/unit/local/remote_document_cache.test.ts

Line 168 in 3223572

genericRemoteDocumentCacheTests();

This can of course be done in a separate PR.

I filed b/117272361 to do it across all clients

schmidt-sebastian · 2018-10-01T18:48:06Z

packages/firestore/test/unit/local/local_store.test.ts

      .finish();
  });

  it('collects garbage after ChangeBatch with no target ids', () => {
+    if (!gcIsEager) {


Same comment about moving this to LocalStore w/ Memory Persistence.

schmidt-sebastian · 2018-10-01T18:50:15Z

packages/firestore/test/unit/specs/spec_test_runner.ts

-      garbageCollector
+    this.persistence = await this.initPersistence(
+      this.serializer,
+      this.useGarbageCollection


Don't we always "use" garbage collection - we just switch between modes?

Kinda. But for the purposes of spec tests, lru garbage collection is no garbage collection, since there is, at least currently, no hook to collect any garbage.

gsoltis · 2018-10-02T17:47:04Z

Whew, ok, I think it's straightened out now.

schmidt-sebastian

I still feel we can improve our waitFor/forEach/waitForEach story.

Can you also comment on the nits I left on the tests? I just want to make sure you saw them. Thanks!

gsoltis · 2018-10-03T22:42:08Z

Sorry, local_store.test.ts comments got hidden. Will address those shortly.

schmidt-sebastian · 2018-10-04T16:48:15Z

packages/firestore/src/local/persistence_promise.ts

+   * Given an iterable, call the given function on each element in the
+   * collection and wait for all of the resulting concurrent
+   * PersistencePromises to resolve.
+   */


I would make it more explicit that it runs in parallel:

/** * Given an iterable, call the given function on each element in * parallel and wait for all of the resulting concurrent * PersistencePromises to resolve. */

I think your update is actually subtly wrong. The callback is called on each element in sequence, not in parallel. The resulting promises run in parallel.

schmidt-sebastian · 2018-10-04T16:51:36Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -643,22 +639,25 @@ function genericLocalStoreTests(
  });

  it('handles SetMutation -> Ack -> PatchMutation -> Reject', () => {


I was hoping that you could just move the tests method to the respective class, similar to what we do here:

firebase-js-sdk/packages/firestore/test/unit/local/remote_document_cache.test.ts

Line 168 in 3223572

genericRemoteDocumentCacheTests();

This can of course be done in a separate PR.

* Implement IndexedDB LRU Reference Delegate, add LRU tests (#1224) * Implement LRU Reference Delegate for Memory Persistence (#1237) * Implement Eager Reference Delegate for Memory Persistence, drop old GC (#1256) * Remove sequential forEach, replace with parallel forEach

Greg Soltis added 5 commits September 21, 2018 11:57

Implement eager delegate

d075850

Removing old garbage collector and fixing tests

bb5cfe6

Merge in lru-master

217b374

Lint

c49cb81

Comments, cleanup

7ffec01

gsoltis assigned schmidt-sebastian Sep 26, 2018

gsoltis requested a review from schmidt-sebastian September 26, 2018 22:46

gsoltis requested review from mikelehen and rsgowman as code owners September 26, 2018 22:46

google-oss-bot added the needs-triage label Sep 26, 2018

gsoltis requested review from var-const, wilhuff and zxu123 as code owners September 26, 2018 22:46

schmidt-sebastian mentioned this pull request Oct 1, 2018

Allow iterables in PersistencePromise.map/forEach/waitFor #1269

Merged

schmidt-sebastian reviewed Oct 1, 2018

View reviewed changes

schmidt-sebastian assigned gsoltis and unassigned schmidt-sebastian Oct 1, 2018

Greg Soltis added 2 commits October 2, 2018 09:41

Merge in master

fc34e9d

Review feedback

d0d3bca

gsoltis assigned schmidt-sebastian and unassigned gsoltis Oct 2, 2018

gsoltis requested review from bojeil-google, davideast, depoll, dwoffinden, Feiyang1, gauntface, hiranya911, mmermerkaya and pinarx as code owners October 2, 2018 17:27

gsoltis removed request for dwoffinden, mmermerkaya, ryanpbrewster, hiranya911, Feiyang1, davideast, var-const, wilhuff, rsgowman, bojeil-google, pinarx, zxu123, wti806 and mikelehen October 2, 2018 17:46

schmidt-sebastian reviewed Oct 3, 2018

View reviewed changes

schmidt-sebastian assigned gsoltis and unassigned schmidt-sebastian Oct 3, 2018

Remove sequential forEach, replace with parallel forEach

d5e3081

Greg Soltis added 3 commits October 3, 2018 16:13

Move gcIsEager flag into LocalStoreTester

75c4c36

[AUTOMATED]: Prettier Code Styling

7ad0348

Lint

73a22b9

schmidt-sebastian approved these changes Oct 4, 2018

View reviewed changes

schmidt-sebastian unassigned gsoltis Oct 4, 2018

gsoltis merged commit cce601e into lru-master Oct 4, 2018

gsoltis deleted the gsoltis/eager_delegate branch October 4, 2018 17:19

gsoltis mentioned this pull request Oct 4, 2018

Merge LRU tracking PRs #1285

Merged

firebase locked and limited conversation to collaborators Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gsoltis commented Sep 26, 2018

schmidt-sebastian left a comment

schmidt-sebastian Oct 1, 2018

gsoltis Oct 2, 2018

schmidt-sebastian Oct 3, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 2, 2018

schmidt-sebastian Oct 3, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 3, 2018

schmidt-sebastian Oct 4, 2018

gsoltis Oct 4, 2018

schmidt-sebastian Oct 1, 2018

schmidt-sebastian Oct 1, 2018

gsoltis Oct 2, 2018

gsoltis commented Oct 2, 2018

schmidt-sebastian left a comment

gsoltis commented Oct 3, 2018

schmidt-sebastian Oct 4, 2018

gsoltis Oct 4, 2018

schmidt-sebastian Oct 4, 2018

		@@ -643,22 +639,25 @@ function genericLocalStoreTests(
		});

		it('handles SetMutation -> Ack -> PatchMutation -> Reject', () => {

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

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

Conversation

gsoltis commented Sep 26, 2018

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoltis commented Oct 2, 2018

schmidt-sebastian left a comment

Choose a reason for hiding this comment

gsoltis commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment