Implement IndexedDB LRU Reference Delegate, add LRU tests #1224

gsoltis · 2018-09-12T18:54:13Z

Adds:

LruGarbageCollector which implements the steps for LRU garbage collection
- Includes a selective port of java.util.PriorityQueue
LruDelegate interface, which specifies what is needed for a persistence layer for LRU GC
ReferenceDelegate interface, which handles lifecycle hooks for document references and unreferences
- Note that this is not yet wired up in most production code, as there is currently only a single implementation
IndexedDbLruDelegate, implements ReferenceDelegate and LruDelegate for IndexedDbPersistence
The LRU test suite
- This currently only runs against IndexedDbPersistence, and as such contains a few casts that will be removed once the generic interfaces support the needed methods.

Android Port: https://github.com/FirebasePrivate/firebase-android-sdk/pull/190
iOS Port: firebase/firebase-ios-sdk#1600

gsoltis · 2018-09-12T18:59:18Z

Oops, lint errors. Will assign back shortly.

gsoltis · 2018-09-12T19:56:03Z

Ok, lint stuff fixed

mikelehen

I flagged a bunch of nits that you can push back on, and may have found one issue. Despite the amount of feedback (sorry), I think this generally looks good. :-)

packages/firestore/src/local/indexeddb_persistence.ts

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

…tor params

…legate

…db_mutation_queue.ts

…istence object

gsoltis · 2018-09-18T16:04:05Z

@mikelehen fyi, I'm also going to create an lru-master branch to merge this into, so the final merge to master will be one commit, like the other clients.

mikelehen

Thanks! A couple potential nits and I'm still concerned about SentinelRow / sentinelKeyStore() being defined as distinct from the TargetDocuments schema in indexeddb_schema.ts, but otherwise looking good.

packages/firestore/src/local/lru_garbage_collector.ts

packages/firestore/src/local/indexeddb_persistence.ts

packages/firestore/src/local/indexeddb_query_cache.ts

packages/firestore/src/local/lru_garbage_collector.ts

mikelehen · 2018-09-18T21:52:13Z

packages/firestore/src/local/indexeddb_persistence.ts

-    });
-    return this.db.getQueryCache().updateQueryData(txn, updated);
+  getTargetCount(txn: PersistenceTransaction): PersistencePromise<number> {
+    return this.db.getQueryCache().getQueryCount(txn);


Do you want to change this to getTargetCount() too, or is this a preexisting issue?

packages/firestore/src/local/persistence.ts

mikelehen · 2018-09-18T22:26:52Z

packages/firestore/src/local/indexeddb_query_cache.ts

+          index: DbTargetDocument.documentTargetsIndex
+        },
+        ([targetId, docKey], { path, sequenceNumber }) => {
+          if (targetId === 0) {


Oh! I assumed that only documents not belonging to a target would have a sentinel row. But I guess every document will have a sentinel row? Is the sequence number for a non-0 targetId row still meaningful? I assumed only the sequence number on the DbTarget row would matter if a document belongs to a target. Yes?

Anyway, I think these questions should be answered in comments in the code. And I think indexeddb_schema.ts is the right place to do that. And to do that, we'll need to include the sentinel row schema in indexeddb_schema.ts as I've requested elsewhere. :-)

mikelehen · 2018-09-18T22:46:12Z

packages/firestore/src/local/indexeddb_persistence.ts

+/**
+ * The sentinel key store is the same as the DbTargetDocument store, but allows for
+ * reading and writing sequence numbers as part of the value stored.
+ */


I feel pretty strongly that indexeddb_schema.ts should continue to be a complete and authoritative description of our schema. So sequenceNumber should be included there.

Having sentinelKey() and sentinelRow() helpers here seems useful and good (and consistent with Android), but sentinelKeyStore() strongly communicates to me that there's a separate object store for these sentinel rows, which is not what we've actually implemented. I'd be fine with moving them to a separate store, but if they're in the "targetDocuments" object store, calling it something else is confusing to me.

So can we drop SentinelRow and sentinelKeyStore() from here, and just define "sequenceNumber" as part of the DbTargetDocument schema?

mikelehen · 2018-09-18T22:55:10Z

packages/firestore/src/local/indexeddb_persistence.ts

+    docKey: DocumentKey
+  ): PersistencePromise<void> {
+    return PersistencePromise.waitFor([
+      sentinelKeyStore(txn).delete(sentinelKey(docKey)),


I still have mixed feelings about this. I liked having individual object stores owned by a specific component. That way it's easy for me to consider all of the interactions within that store together. I'm curious why we didn't just have a separate store for the sentinel rows, but I assume there was a good reason and that ship has probably sailed anyway.

So I'm fine with this as-is, except per my other comments I think we should avoid sentinelKeyStore() since it suggests there is a separate store, which is not the case.

Swap out priority queue for a sorted set

…et document store

gsoltis

Ok, I think I've addressed everything? Unless github is still hiding things from me...

gsoltis · 2018-09-18T23:30:54Z

packages/firestore/src/local/indexeddb_persistence.ts

+    docKey: DocumentKey
+  ): PersistencePromise<void> {
+    return PersistencePromise.waitFor([
+      sentinelKeyStore(txn).delete(sentinelKey(docKey)),


The reason sentinel rows are interleaved with target-document rows is efficiency. The current algorithm for identifying orphaned documents is a scan. Using a separate store would require scanning that store, and then for each row, doing a lookup in the target-document store. FWIW, we tried this in sqlite on Android by adding a sequence number column to each remote document. It was faster to add a nullable column to the target-document many-to-many table that only had values where targetId == 0.

gsoltis · 2018-09-18T23:37:33Z

packages/firestore/src/local/indexeddb_persistence.ts

+/**
+ * The sentinel key store is the same as the DbTargetDocument store, but allows for
+ * reading and writing sequence numbers as part of the value stored.
+ */


My main concern is that I don't want the query cache to have access to the sequence number. It's not present for any rows that the query cache would be dealing with. I've removed SentinelRow and sentinelKeyStore() though. I've added an assert to the constructor for DbTargetDocument.

packages/firestore/src/local/lru_garbage_collector.ts

packages/firestore/src/local/persistence.ts

packages/firestore/src/local/lru_garbage_collector.ts

mikelehen

LGTM with a couple nits.

packages/firestore/src/local/indexeddb_schema.ts

* 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

Requested rename from firebase/firebase-js-sdk#1224, targets is a better name than queries for internal portions of the SDK.

Implement IndexedDB LRU Reference Delegate, add LRU tests

eab8740

gsoltis assigned mikelehen Sep 12, 2018

gsoltis requested a review from mikelehen September 12, 2018 18:54

gsoltis requested review from rsgowman, schmidt-sebastian, var-const, wilhuff and zxu123 as code owners September 12, 2018 18:54

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

gsoltis assigned gsoltis and unassigned mikelehen Sep 12, 2018

Greg Soltis added 2 commits September 12, 2018 12:14

Lint cleanup

6818502

[AUTOMATED]: Prettier Code Styling

d37c2c4

gsoltis assigned mikelehen and unassigned gsoltis Sep 12, 2018

mikelehen suggested changes Sep 14, 2018

View reviewed changes

mikelehen assigned gsoltis and unassigned mikelehen Sep 14, 2018

Greg Soltis added 9 commits September 17, 2018 10:47

Add garbagecollector property, reorder indexeddb query cache construc…

11cad0d

…tor params

LiveTargets -> ActiveTargets, fix comments on ReferenceDelegate

3114ce7

[AUTOMATED]: Prettier Code Styling

5718c24

Methods reordered to match android

baa9fa8

Remove forEachOrphanedDocument from query cache, move to reference de…

ed3c84d

…legate

Fix comment typo and reflow, move mutationQueuesContainKey to indexed…

9d68610

…db_mutation_queue.ts

[AUTOMATED]: Prettier Code Styling

cba7bf7

Only pass reference delegate to indexeddb query cache, not whole pers…

f88817a

…istence object

[AUTOMATED]: Prettier Code Styling

a79a941

gsoltis mentioned this pull request Sep 17, 2018

removeQueries -> removeTargets firebase/firebase-android-sdk#23

Merged

Greg Soltis added 2 commits September 17, 2018 16:35

Regroup tests a little

279e14a

A little test cleanup

34f7967

gsoltis assigned mikelehen and unassigned gsoltis Sep 17, 2018

gsoltis changed the base branch from master to lru-master September 18, 2018 16:16

gsoltis mentioned this pull request Sep 18, 2018

Using SortedSet instead of a PriorityQueue #1233

Merged

mikelehen suggested changes Sep 18, 2018

View reviewed changes

mikelehen assigned gsoltis and unassigned mikelehen Sep 18, 2018

Greg Soltis added 4 commits September 18, 2018 16:43

additionalReferences -> inMemoryPins

801694c

Using SortedSet instead of a PriorityQueue (#1233)

34f62fb

Swap out priority queue for a sorted set

Drop sentinel row and sentinel key store. Add sequence number to targ…

3b748b1

…et document store

Merge

11b0baf

gsoltis commented Sep 19, 2018

View reviewed changes

gsoltis assigned mikelehen and unassigned gsoltis Sep 19, 2018

Lint

366418e

mikelehen approved these changes Sep 19, 2018

View reviewed changes

packages/firestore/src/local/indexeddb_schema.ts Outdated Show resolved Hide resolved

packages/firestore/src/local/indexeddb_schema.ts Outdated Show resolved Hide resolved

mikelehen assigned gsoltis and unassigned mikelehen Sep 19, 2018

Greg Soltis added 2 commits September 19, 2018 11:16

Comments and flip conditional

fcbb366

Compare to undefined, not typeof undefined

c922dce

gsoltis merged commit eaeeb2a into lru-master Sep 19, 2018

gsoltis deleted the gsoltis/indexeddb_lru branch September 19, 2018 18:46

gsoltis mentioned this pull request Sep 19, 2018

Renaming from webclient review firebase/firebase-android-sdk#31

Merged

gsoltis mentioned this pull request Oct 4, 2018

Merge LRU tracking PRs #1285

Merged

long1eu added a commit to long1eu/firebase_dart_sdk that referenced this pull request Oct 15, 2018

removeQueries -> removeTargets (#23)

459389a

Requested rename from firebase/firebase-js-sdk#1224, targets is a better name than queries for internal portions of the SDK.

firebase locked and limited conversation to collaborators Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IndexedDB LRU Reference Delegate, add LRU tests #1224

Implement IndexedDB LRU Reference Delegate, add LRU tests #1224

gsoltis commented Sep 12, 2018

gsoltis commented Sep 12, 2018

gsoltis commented Sep 12, 2018

mikelehen left a comment

gsoltis commented Sep 18, 2018

mikelehen left a comment

mikelehen Sep 18, 2018

mikelehen Sep 18, 2018

mikelehen Sep 18, 2018

mikelehen Sep 18, 2018

gsoltis left a comment

gsoltis Sep 18, 2018

gsoltis Sep 18, 2018

mikelehen left a comment

Implement IndexedDB LRU Reference Delegate, add LRU tests #1224

Implement IndexedDB LRU Reference Delegate, add LRU tests #1224

Conversation

gsoltis commented Sep 12, 2018

gsoltis commented Sep 12, 2018

gsoltis commented Sep 12, 2018

mikelehen left a comment

Choose a reason for hiding this comment

gsoltis commented Sep 18, 2018

mikelehen left a comment

Choose a reason for hiding this comment

mikelehen Sep 18, 2018

Choose a reason for hiding this comment

mikelehen Sep 18, 2018

Choose a reason for hiding this comment

mikelehen Sep 18, 2018

Choose a reason for hiding this comment

mikelehen Sep 18, 2018

Choose a reason for hiding this comment

gsoltis left a comment

Choose a reason for hiding this comment

gsoltis Sep 18, 2018

Choose a reason for hiding this comment

gsoltis Sep 18, 2018

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment