Use Read-Time Index for Multi-Tab #2128

schmidt-sebastian · 2019-08-30T01:20:20Z

This uses the read time we persist for Index-Free queries in the change log for multi-tab. The big benefit is that we no longer need to GC the change log (since LRU GC does it for us) and we no longer need to recover from GCed change logs. The downside is that we now need to store NoDocuments for deleted rows, but these are internal to the IndexedDbRemoteDocumentStore (I added a new option to the RemoteDocumentChangeBuffer for that).

This is a follow-up to #2125

schmidt-sebastian · 2019-08-30T01:23:06Z

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

@@ -1144,7 +1144,7 @@ describeSpec('Listens:', [], () => {
      .client(1)
      .stealPrimaryLease()
      .expectListen(query, 'resume-token-2000')
-      .watchAcksFull(query, 2000, docC)
+      .watchAcksFull(query, 3000, docC)


This changed since we can use the read time to get a change diff, rather than a separate counter.

This should fix the CI failure in #2128

mikelehen

Mostly LGTM but a few nits and I think we should sanity-check with @wilhuff that we're okay with web storing sentinel delete documents that iOS / Android do not.

mikelehen · 2019-08-31T00:16:56Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -1237,7 +1211,9 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
    upperBound: ListenSequenceNumber
  ): PersistencePromise<number> {
    const documentCache = this.db.getRemoteDocumentCache();
-    const changeBuffer = documentCache.newChangeBuffer();
+    const changeBuffer = documentCache.newChangeBuffer({
+      createSentinelDocumentsToTrackDeletes: false


Perhaps change "Deletes" => "Removals" or "RemovedEntries" since this corresponds to removeEntry() calls.

Done (trackRemovals as per later comment)

packages/firestore/src/local/indexeddb_schema.ts

packages/firestore/src/local/indexeddb_remote_document_cache.ts

mikelehen · 2019-09-03T16:25:12Z

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -486,15 +447,22 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
          promises.push(this.documentCache.addEntry(transaction, key, doc));
        } else {
          sizeDelta -= previousSize!;
-          promises.push(this.documentCache.removeEntry(transaction, key));
+          if (this.trackDeletes) {
+            // A sentinel delete is symbolized by a NoDocument with a version of 0.


Maybe expand on this comment?

In order to track deletes, we store "sentinel delete" entries in the Remote Document Cache. These entries are represented by a NoDocument with a version of 0 and are ignored by maybeDecodeDocument() but preserved in getNewDocumentChanges().

Done. Thanks for the comment :)

mikelehen · 2019-09-03T16:28:00Z

packages/firestore/src/local/indexeddb_remote_document_cache.ts

-  newChangeBuffer(): RemoteDocumentChangeBuffer {
-    return new IndexedDbRemoteDocumentCache.RemoteDocumentChangeBuffer(this);
+  newChangeBuffer(options: {
+    createSentinelDocumentsToTrackDeletes: boolean;


You say "sentinelDocuments" here but refer to them as "sentinel deletes" elsewhere. Please use consistent terminology.

We have this long name here but just trackDeletes in the RemoteDocumentChangeBuffer constructor. I am not sure which I prefer, but I can't think of a great reason for them to be different.

Since you seem to be ok with the shorter name, I opted for "trackRemovals" everywhere.

packages/firestore/src/local/indexeddb_schema.ts

mikelehen · 2019-09-03T16:44:30Z

packages/firestore/src/local/indexeddb_schema.ts

-  DbClientMetadata.store,
-  DbRemoteDocumentChanges.store
-];
+export const V4_STORES = [...V3_STORES, DbClientMetadata.store];


Hrm... I'm having uneasy feelings about revising history like this. I wonder if we should keep some hint about the prior existence of this store to keep our history of schema changes in-tact in this file. I'm not sure though. It also seems strange to keep dead code. Maybe ask @wilhuff what he thinks.

As per our offline discussion, let's keep this as is. I added a comment about the prior existence of this store. Theoretically though it should not matter since we drop the store, so we can recreate it with the same name and use for a different purpose later.

mikelehen · 2019-09-03T16:47:06Z

packages/firestore/src/local/local_store.ts

@@ -345,7 +345,9 @@ export class LocalStore {
      'readwrite-primary',
      txn => {
        const affected = batchResult.batch.keys();
-        const documentBuffer = this.remoteDocuments.newChangeBuffer();
+        const documentBuffer = this.remoteDocuments.newChangeBuffer({
+          createSentinelDocumentsToTrackDeletes: true


Part of me wonders if this should be the default behavior and GC should explicitly opt-out (e.g. flip the logic and rename to suppressSentinelDeleteDocuments or something) since reading this code, I would have trouble guessing what this is for and why it is enabled in this code path. By contrast, the GC code could have a comment explaining why we don't want to track deletes in that case.

Alternatively, since this option probably corresponds 100% to whether this is GC code or not, we could consider specializing the name to match (e.g. forGarbageCollection: true or something)

WDYT?

I'm a little torn on this. I think the current naming makes the behavior more explicit (compared to forGarbageCollection) and I am not a big fan of flipping the behavior since we are turning on an extra feature after all. I do see your point though and can probably be won over somewhat easily.

For now, I made the argument optional and added a comment in the two places that requires us to turn trackRemovals on.

mikelehen · 2019-09-03T16:51:01Z

packages/firestore/src/local/memory_remote_document_cache.ts

@@ -177,7 +178,9 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache {
    return PersistencePromise.resolve(changedDocs);
  }

-  newChangeBuffer(): RemoteDocumentChangeBuffer {
+  newChangeBuffer(options: {
+    createSentinelDocumentsToTrackDeletes: boolean;


Perhaps there should be a comment indicating we explicitly ignore this.

Added. This makes me thing we should never have added the change log to the MemoryRemoteDocumentCache.

mikelehen · 2019-09-03T16:54:59Z

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

+                }
+              )
+              .next(() => {
+                // The old documents are not included since readTime was not populated


Would be nice if this test added some documents with index entries but that were older than version=1 to verify that are not included either...

I extracted this into a separate test ("can get recent document changes") that does.

* Update typescript-eslint monorepo to v2 * fix breaking changes

This should fix the CI failure in #2128

schmidt-sebastian · 2019-09-04T19:13:28Z

I used "Sebastian's guide to screw with your commit history for this PR". ~~The review changes are part of 8142123~~

Ugh, I totally screwed this up. Do you mind reviewing the full diff again? I also had to add the readTime as an argument for removeEntry() in order to serialize a proper NoDocument event.

mikelehen

Mostly LGTM but in order to be consistent with my previous feedback, I'd like to see if we can remove the version number from removeEntry()... again. 😬

mikelehen · 2019-09-04T21:15:13Z

packages/firestore/src/local/indexeddb_persistence.ts

+                changeBuffer.removeEntry(
+                  docKey,
+                  SnapshotVersion.forDeletedDoc()
+                );


Per chat conversation, I will not stand for removeEntry() requiring a version that is MEANINGLESS!!! j/k... But I am realizing that there is exactly one place where we want to track removed entries and that is for failed limbo resolution, so I'm wondering if we should:

Remove the trackRemovals option.

Make it so removeEntry() doesn't require a version.

Let you optionally pass an entry in. This could be removeEntry(key, version) or if we want to make it a little more explicit (and borrow a bit from the existing shape of the API), maybe removeEntry(key, {trackRemovalVersion: version}) or something.

Alternatively, since this such a specific case, we could consider not storing the sentinel delete and inventing a localStorage notification for it instead.

As always, this didn't quite work out in practice :/ If I do want to do this, then I have to track the readTime per document. If our semantic are going to be "when a read time is provided, create a fake NoDocument" then we should allow for the fact that some removals have readTimes while others do not.

I still made the second argument optional. If no read time is provided when it is needed, the assert in get readTime() should provide some insight.

mikelehen · 2019-09-04T21:17:36Z

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -131,24 +110,18 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
  }

  /**
-   * Updates the document change log and adds the given delta to the cached current size.
+   * Updates the current size.


maybe "current cache size" ?

mikelehen · 2019-09-04T21:41:16Z

packages/firestore/src/local/indexeddb_remote_document_cache.ts

-  newChangeBuffer(): RemoteDocumentChangeBuffer {
-    return new IndexedDbRemoteDocumentCache.RemoteDocumentChangeBuffer(this);
+  newChangeBuffer(options?: {
+    trackRemovals: boolean;


Per feedback elsewhere, I recommend dropping this option and "moving" it to the removeEntry() call.

mikelehen · 2019-09-04T21:44:17Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -617,6 +630,12 @@ export class DbRemoteDocument {

  static collectionReadTimeIndexPath = ['parentPath', 'readTime'];

+  // TODO: We are currently storing full document keys almost three times
+  // (once as part of the primary key, once - partly - as `parentPath` and once
+  // inside the encoded documents). During our next migration, we should


We store it 3 more times if you include the indexes (collectionReadTimeIndex stores it twice, readTimeIndex stores it once). Hopefully people don't use deeply-nested subcollections and small documents!

It occurs to me that we could maintain our own collectionReadTimeIndex that instead of being a true index was just an object store containing {collectionPath, readTime, documentId} objects. This would save us storing the full documentKey. It's probably not worth doing for web since it'd be less efficient than a real index, but with LevelDB (where we have to implement our own indexes anyway), it probably makes sense.

So much redundancy! So much uptime!

I am planning on using that format for LevelDB, but I prefer using existing functionality here even if it is slightly less storage efficient.

mikelehen

LGTM after merging #2145

schmidt-sebastian added 2 commits August 29, 2019 18:10

Track readTime in the RemoteDocument store

fd5678b

Use Read-Time Index for Multi-Tab

1c0e369

schmidt-sebastian commented Aug 30, 2019

View reviewed changes

schmidt-sebastian requested a review from mikelehen August 30, 2019 01:23

schmidt-sebastian assigned mikelehen Aug 30, 2019

Use sentinel doc logic everywhere

00f1ca8

schmidt-sebastian added a commit that referenced this pull request Aug 30, 2019

Upgrade Firestore Emulator to 1.8.2

ec5e532

This should fix the CI failure in #2128

schmidt-sebastian added a commit that referenced this pull request Aug 30, 2019

Upgrade Firestore Emulator to 1.8.2

1dd67ec

This should fix the CI failure in #2128

schmidt-sebastian mentioned this pull request Aug 30, 2019

Upgrade Firestore Emulator to 1.8.2 #2133

Merged

Rewrap comment

c75dab6

mikelehen pushed a commit that referenced this pull request Aug 30, 2019

Upgrade Firestore Emulator to 1.8.2 (#2133)

16253f6

This should fix the CI failure in #2128

mikelehen suggested changes Sep 3, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 3, 2019

mikelehen added the api: firestore label Sep 3, 2019

renovate-bot and others added 8 commits September 4, 2019 12:00

chore(deps): update typescript-eslint monorepo to v2 (major) (#2087)

919b1eb

* Update typescript-eslint monorepo to v2 * fix breaking changes

Publish [email protected]

d59c85c

Upgrade Firestore Emulator to 1.8.2 (#2133)

7cb9e46

This should fix the CI failure in #2128

Address feedback

cf91ba6

Fix test

abf072d

Sort some imports

97f98a2

Comments

3426035

Index-Free: Track readTime in the RemoteDocument store (#2125)

b818ec9

schmidt-sebastian force-pushed the mrschmidt/multitabreadtime branch from 4879484 to fc9ed49 Compare September 4, 2019 19:06

schmidt-sebastian requested review from alikn, andirayo, davideast, dwoffinden and Feiyang1 as code owners September 4, 2019 19:06

schmidt-sebastian requested a review from zijianjoy as a code owner September 4, 2019 19:06

schmidt-sebastian changed the base branch from mrschmidt/persistreadtime to mrschmidt/indexfree-master September 4, 2019 19:07

Review feedback

8142123

schmidt-sebastian force-pushed the mrschmidt/multitabreadtime branch from fc9ed49 to 8142123 Compare September 4, 2019 19:09

Merge branch 'master' into mrschmidt/multitabreadtime

5cb599d

schmidt-sebastian requested review from bojeil-google, jsdt and wti806 as code owners September 4, 2019 19:10

Merge

41cf21b

schmidt-sebastian added 3 commits September 4, 2019 12:34

Review feedback

500b555

new line

935eae3

[AUTOMATED]: Prettier Code Styling

5448dff

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Sep 4, 2019

mikelehen suggested changes Sep 4, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 4, 2019

Almost address feedback

aee3d0c

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Sep 4, 2019

mikelehen approved these changes Sep 4, 2019

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Sep 4, 2019

Michael Lehenbauer and others added 3 commits September 4, 2019 16:39

Misc. fixes. (#2145)

d23d134

Fix test. (#2147)

d6b5866

Add Emulator workaround

f7211db

schmidt-sebastian merged commit 06e6c50 into mrschmidt/indexfree-master Sep 5, 2019

firebase locked and limited conversation to collaborators Oct 8, 2019

schmidt-sebastian deleted the mrschmidt/multitabreadtime branch October 10, 2019 22:55

Use Read-Time Index for Multi-Tab #2128

Use Read-Time Index for Multi-Tab #2128

Uh oh!

Conversation

schmidt-sebastian commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Aug 30, 2019 •

edited

Loading

schmidt-sebastian Sep 4, 2019 •

edited

Loading

schmidt-sebastian commented Sep 4, 2019 •

edited

Loading