Port optimizations to LocalDocumentsView from iOS #1055

var-const · 2018-07-27T01:25:41Z

add a method to find batches affecting a set of keys (port of 1479);
use the newly-added method to avoid rereading batches when getting documents in LocalDocumentsView (port of 1505);
avoid rereading batches when searching for documents in a collection (port of 1533).

mikelehen

Mostly good, but I think this port looks close to the beginning state of Android instead of the final state (we have O(mutation queue) implementations instead of O(documentKeys))

mikelehen · 2018-07-27T19:54:50Z

packages/firestore/src/local/indexeddb_mutation_queue.ts

+          return;
+        }
+        uniqueBatchIDs = uniqueBatchIDs.add(batchID);
+      })


So I think this is O(mutation queue size) instead of O(documentKeys size)... but can probably be fixed to be O(documentKeys size) by either calling iterate again for each documentKey, or by using control.skipToKey() after finding / not-finding each documentKey.

mikelehen · 2018-07-27T20:11:58Z

packages/firestore/src/local/local_documents_view.ts

+  }
+
+  /** Internal version of `getDocument` that allows reusing batches. */
+  private getDocumentInBatches(


Not wild about getDocumentInBatches(). I'd probably rename to getDocumentInternal(). Or you could just combine with getDocument() and have inBatches be an optional param:

getDocument( transaction: PersistenceTransaction, key: DocumentKey, inBatches?: MutationBatch[]) { let inBatchesPromise = inBatches ? PersistencePromise.resolve(inBatches) : this.mutationQueue.getAllMutationBatchesAffectingDocumentKey(transaction, key); return inBatchesPromise.next(inBatches => this.remoteDocumentCache.getEntry(transaction, key)) .next(doc => { for (const batch of inBatches) { doc = batch.applyToLocalView(key, doc); } return doc; }); }

(written via copy/paste and untested so probably not quite right)

Do you have any preference in favor of the optional param? I'm fine with that, but it would be a little less consistent with other platforms, where the other function is private. OTOH, perhaps it's more idiomatic?

No preference. You're right that it would ideally be private... but TypeScript doesn't support "overloads" with mixed visibility. So I think getDocumentInternal() is good.

mikelehen · 2018-07-27T20:24:48Z

packages/firestore/src/local/memory_mutation_queue.ts

+      if (documentKeys.has(ref.key)) {
+        uniqueBatchIDs = uniqueBatchIDs.add(ref.targetOrBatchId);
+      }
+    });


Again, this is O(mutation queue) instead of O(document keys). Can we make it match iOS / Android?

mikelehen · 2018-07-27T20:27:02Z

packages/firestore/src/local/mutation_queue.ts

+   * convenient.
+   */
+  // TODO(mcg): This should really return an enumerator
+  // also for b/32992024, all backing stores should really index by document key


Hrm. b/32992024 is closed and I think this comment is obsolete (documentMutations is our index). Can you remove it here and above?

…nst/port-affecting-batches

mikelehen

LGTM with optional nit. Thanks!

mikelehen · 2018-07-30T17:15:59Z

packages/firestore/src/local/memory_mutation_queue.ts

+      const end = new DocReference(documentKey, Number.POSITIVE_INFINITY);
+      this.batchesByDocumentKey.forEachInRange([start, end], ref => {
+        if (!documentKey.isEqual(ref.key)) {
+          return;


nit: Is it possible to hit this? Seems like our range should constrain us to documentKey.

I'm not sure. This function is derived from getAllMutationBatchesAffectingDocumentKey, which asserts this equality, this led me to believe this case is not impossible. I can remove it altogether or replace with an assertion.

Asserts should generally mean the case is impossible. They exist to document our expectations and/or catch false expectations as early as possible to make finding / fixing bugs later. So I think an assert here makes perfect sense.

Fine print: We're probably not 100% rigorous about asserts always being impossible. E.g. there may be cases where we use asserts for cases that /are/ possible but represent rare error conditions that we don't feel compelled to provide proper error handling for.

Replicated the assertion.

mikelehen · 2018-07-30T17:17:30Z

packages/firestore/src/local/local_documents_view.ts

+  }
+
+  /** Internal version of `getDocument` that allows reusing batches. */
+  private getDocumentInBatches(


No preference. You're right that it would ideally be private... but TypeScript doesn't support "overloads" with mixed visibility. So I think getDocumentInternal() is good.

var-const

Did some performance tests (by running yarn test:browser with persistence enabled (see code below). The test consisted of writing 10 batches of 500 mutations each, where each mutation creates a very small document in the same collection (so the resulting collection contains 5K documents). Ran the test on master and in this branch 3 times each. Results (in milliseconds):

writing batches in master: 37169 38600 38246
writing batches in branch: 11282 14307 14774
reading the collection in master: 30784 5189 5858
reading the collection in branch: 1124 851 844.

One weird thing I noticed is that for any given project, reading the collection for the first time is slower than on subsequent runs of the test (all tests run with --single_run). The difference is really noticeable on master (31 vs 5-6 seconds) and less pronounced in this branch (1120 ms vs 850 ms). Changing the project after a "fast" read will result in one "slow" read, but the subsequent runs are "fast" again. The reading is done in offline mode, so this is a little confusing.

The ad-hoc code I used for the test:

   let colRef = null;
    let writeStartTime = 0;
    let readStartTime = 0;

    const promises = [];
    return withTestCollection(persistence, {}, coll => {
      colRef = coll;
      colRef.onSnapshot(() => {});

      writeStartTime = Date.now();

      for (let iter = 0; iter !== 10; ++iter) {
        const batch = colRef.firestore.batch();
        for (let i = 0; i !== 500; ++i) {
          batch.set(colRef.doc(), { foo: 'bar' + i });
        }
        promises.push(batch.commit());
      }

      return Promise.all(promises)
    .then(() => {
        const writeEndTime = Date.now();
        console.error('Writing batches took ' + (writeEndTime - writeStartTime) + ' ms');
        return colRef.firestore.disableNetwork();
    })
    .then(() => {
      readStartTime = Date.now();
      return colRef.get({source: 'cache'});
    })
    .then(qrySnap => {
      const readEndTime = Date.now();
      console.error(
        'Get collection took: ' + (readEndTime - readStartTime) + ' ms'
      );
    });
  });

var-const · 2018-07-31T02:06:59Z

packages/firestore/src/local/memory_mutation_queue.ts

+      const end = new DocReference(documentKey, Number.POSITIVE_INFINITY);
+      this.batchesByDocumentKey.forEachInRange([start, end], ref => {
+        if (!documentKey.isEqual(ref.key)) {
+          return;


I'm not sure. This function is derived from getAllMutationBatchesAffectingDocumentKey, which asserts this equality, this led me to believe this case is not impossible. I can remove it altogether or replace with an assertion.

mikelehen · 2018-07-31T16:02:27Z

Nice! Thanks for the performance data.

I can't 100% explain the "first time slow" issue without digging in, but it may be related to the "colRef.onSnapshot(() => {});" line... That's going to spin up a full normal listener against the backend which involves reading documents and query metadata from persistence, sending the result to the backend, and handling the response data.

To cancel that out, you could make sure the listener has fully initialized / settled by doing something like:

  ...
  let deferred = new Deferred();
  colRef.onSnapshot(snap => if (!snap.metadata.fromCache) deferred.resolve());
  return deferred.promise;
}).then(() => {
  writeStartTime = Date.now();
  ...

…/firebase-js-sdk into varconst/port-affecting-batches

var-const

Thanks for the explanation! Adding the snippet you suggested indeed eliminated the first-time slowdown, now I get consistent (faster) results. So my tests with 10 batches show 3x speedup in writing batches and ~6x speedup reading large collections (presumably even more so for more batches).

var-const · 2018-07-31T17:07:17Z

packages/firestore/src/local/memory_mutation_queue.ts

+      const end = new DocReference(documentKey, Number.POSITIVE_INFINITY);
+      this.batchesByDocumentKey.forEachInRange([start, end], ref => {
+        if (!documentKey.isEqual(ref.key)) {
+          return;


Replicated the assertion.

mikelehen

Still LGTM. Thanks!

var-const · 2018-07-31T18:05:13Z

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

@@ -433,7 +434,30 @@ function genericMutationQueueTests(): void {
    const matches = await mutationQueue.getAllMutationBatchesAffectingDocumentKey(
      key('foo/bar')
    );
-    expect(matches.length).to.deep.equal(expected.length);


Forgot to mention: I noticed that expectEqualArrays checks for equal length, so this line seems redundant.

* Add a CHANGELOG entry for #1052 * Add notes for #1055

* Implement global resume token (#1052) * Add a spec test that shows correct global resume token handling * Minimum implementation to handle global resume tokens * Remove unused QueryView.resumeToken * Avoid persisting the resume token unless required * Persist the resume token on unlisten * Add a type parameter to Persistence (#1047) * Cherry pick sequence number starting point * Working on typed transactions * Start plumbing in sequence number * Back out sequence number changes * [AUTOMATED]: Prettier Code Styling * Fix tests * [AUTOMATED]: Prettier Code Styling * Fix lint * [AUTOMATED]: Prettier Code Styling * Uncomment line * MemoryPersistenceTransaction -> MemoryTransaction * [AUTOMATED]: Prettier Code Styling * Review updates * Style * Lint and style * Review feedback * [AUTOMATED]: Prettier Code Styling * Revert some unintentional import churn * Line 44 should definitely be empty * Checkpoint before adding helper function for stores * Use a helper for casting PersistenceTransaction to IndexedDbTransaction * [AUTOMATED]: Prettier Code Styling * Remove errant generic type * Lint * Fix typo * Port optimizations to LocalDocumentsView from iOS (#1055) * add a method to find batches affecting a set of keys (port of [1479](firebase/firebase-ios-sdk#1479)); * use the newly-added method to avoid rereading batches when getting documents in `LocalDocumentsView` (port of [1505](firebase/firebase-ios-sdk#1505)); * avoid rereading batches when searching for documents in a collection (port of [1533](firebase/firebase-ios-sdk#1533)). Speedup was measured by running tests in browser and checking time spent writing 10 batches of 500 mutations each, and then querying the resulting 5K docs collection from cache in offline mode. For this case, the writing speedup is about 3x, and querying speedup is about 6x (see PR for more details). * Add a CHANGELOG entry for #1052 (#1071) * Add a CHANGELOG entry for #1052 * Add notes for #1055 * Rename idleTimer and fix comments. (#1068) * Merge (#1073)

* Merging PersistentStream refactor * [AUTOMATED]: Prettier Code Styling * Typo * Remove canUseNetwork state. (#1076) * Merging the latest merge into the previous merge (#1077) * Implement global resume token (#1052) * Add a spec test that shows correct global resume token handling * Minimum implementation to handle global resume tokens * Remove unused QueryView.resumeToken * Avoid persisting the resume token unless required * Persist the resume token on unlisten * Add a type parameter to Persistence (#1047) * Cherry pick sequence number starting point * Working on typed transactions * Start plumbing in sequence number * Back out sequence number changes * [AUTOMATED]: Prettier Code Styling * Fix tests * [AUTOMATED]: Prettier Code Styling * Fix lint * [AUTOMATED]: Prettier Code Styling * Uncomment line * MemoryPersistenceTransaction -> MemoryTransaction * [AUTOMATED]: Prettier Code Styling * Review updates * Style * Lint and style * Review feedback * [AUTOMATED]: Prettier Code Styling * Revert some unintentional import churn * Line 44 should definitely be empty * Checkpoint before adding helper function for stores * Use a helper for casting PersistenceTransaction to IndexedDbTransaction * [AUTOMATED]: Prettier Code Styling * Remove errant generic type * Lint * Fix typo * Port optimizations to LocalDocumentsView from iOS (#1055) * add a method to find batches affecting a set of keys (port of [1479](firebase/firebase-ios-sdk#1479)); * use the newly-added method to avoid rereading batches when getting documents in `LocalDocumentsView` (port of [1505](firebase/firebase-ios-sdk#1505)); * avoid rereading batches when searching for documents in a collection (port of [1533](firebase/firebase-ios-sdk#1533)). Speedup was measured by running tests in browser and checking time spent writing 10 batches of 500 mutations each, and then querying the resulting 5K docs collection from cache in offline mode. For this case, the writing speedup is about 3x, and querying speedup is about 6x (see PR for more details). * Add a CHANGELOG entry for #1052 (#1071) * Add a CHANGELOG entry for #1052 * Add notes for #1055 * Rename idleTimer and fix comments. (#1068) * Merge (#1073)

* Catch invalid provider id error (#1064) * RxFire: Api Change and documentation (#1066) * api changes and doc updates * fixes * Refactor PersistentStream (no behavior changes). (#1041) This breaks out a number of changes I made as prep for b/80402781 (Continue retrying streams for 1 minute (idle delay)). PersistentStream changes: * Rather than providing a stream event listener to every call of start(), the stream listener is now provided once to the constructor and cannot be changed. * Streams can now be restarted indefinitely, even after a call to stop(). * PersistentStreamState.Stopped was removed and we just return to 'Initial' after a stop() call. * Added `closeCount` member to PersistentStream in order to avoid bleedthrough issues with auth and stream events once stop() has been called. * Calling stop() now triggers the onClose() event listener, which simplifies stream cleanup. * PersistentStreamState.Auth renamed to 'Starting' to better reflect that it encompasses both authentication and opening the stream. RemoteStore changes: * Creates streams once and just stop() / start()s them as necessary, never recreating them completely. * Added networkEnabled flag to track whether the network is enabled or not, since we no longer null out the streams. * Refactored disableNetwork() / enableNetwork() to remove stream re-creation. Misc: * Comment improvements including a state diagram on PersistentStream. * Fixed spec test shutdown to schedule via the AsyncQueue to fix sequencing order I ran into. * Merging Persistent Stream refactor (#1069) * Merging PersistentStream refactor * [AUTOMATED]: Prettier Code Styling * Typo * Remove canUseNetwork state. (#1076) * Merging the latest merge into the previous merge (#1077) * Implement global resume token (#1052) * Add a spec test that shows correct global resume token handling * Minimum implementation to handle global resume tokens * Remove unused QueryView.resumeToken * Avoid persisting the resume token unless required * Persist the resume token on unlisten * Add a type parameter to Persistence (#1047) * Cherry pick sequence number starting point * Working on typed transactions * Start plumbing in sequence number * Back out sequence number changes * [AUTOMATED]: Prettier Code Styling * Fix tests * [AUTOMATED]: Prettier Code Styling * Fix lint * [AUTOMATED]: Prettier Code Styling * Uncomment line * MemoryPersistenceTransaction -> MemoryTransaction * [AUTOMATED]: Prettier Code Styling * Review updates * Style * Lint and style * Review feedback * [AUTOMATED]: Prettier Code Styling * Revert some unintentional import churn * Line 44 should definitely be empty * Checkpoint before adding helper function for stores * Use a helper for casting PersistenceTransaction to IndexedDbTransaction * [AUTOMATED]: Prettier Code Styling * Remove errant generic type * Lint * Fix typo * Port optimizations to LocalDocumentsView from iOS (#1055) * add a method to find batches affecting a set of keys (port of [1479](firebase/firebase-ios-sdk#1479)); * use the newly-added method to avoid rereading batches when getting documents in `LocalDocumentsView` (port of [1505](firebase/firebase-ios-sdk#1505)); * avoid rereading batches when searching for documents in a collection (port of [1533](firebase/firebase-ios-sdk#1533)). Speedup was measured by running tests in browser and checking time spent writing 10 batches of 500 mutations each, and then querying the resulting 5K docs collection from cache in offline mode. For this case, the writing speedup is about 3x, and querying speedup is about 6x (see PR for more details). * Add a CHANGELOG entry for #1052 (#1071) * Add a CHANGELOG entry for #1052 * Add notes for #1055 * Rename idleTimer and fix comments. (#1068) * Merge (#1073)

var-const added 8 commits July 25, 2018 16:27

wip

65a53b2

Local docs, path

f94e186

memory mutation queue, test left

64a293d

test fix

a969f3b

Fix iteration

307a478

add test

2de3301

[AUTOMATED]: Prettier Code Styling

5bf87ae

small fixes

90a04cb

var-const requested review from mikelehen, schmidt-sebastian and wilhuff as code owners July 27, 2018 01:25

google-oss-bot added the needs-triage label Jul 27, 2018

var-const assigned mikelehen Jul 27, 2018

var-const added api: firestore and removed needs-triage labels Jul 27, 2018

appease linter

b94e7c7

mikelehen suggested changes Jul 27, 2018

View reviewed changes

mikelehen assigned var-const and unassigned mikelehen Jul 27, 2018

var-const added 7 commits July 27, 2018 18:45

Initial

5b24623

[AUTOMATED]: Prettier Code Styling

49a261e

Merge branch 'varconst/silence-timestamp-warning-in-tests' into varco…

27c88eb

…nst/port-affecting-batches

review feedback wip

223c430

Merge branch 'master' into varconst/port-affecting-batches

42bf72b

Review feedback, indexeddb part

c8375a6

Review feedback, memory part

87e0b06

var-const assigned mikelehen and unassigned var-const Jul 30, 2018

mikelehen approved these changes Jul 30, 2018

View reviewed changes

mikelehen removed their assignment Jul 30, 2018

mikelehen assigned var-const Jul 30, 2018

var-const commented Jul 31, 2018

View reviewed changes

Merge branch 'master' into varconst/port-affecting-batches

7082064

var-const added 4 commits July 31, 2018 13:14

Review fix

0d068f5

[AUTOMATED]: Prettier Code Styling

5dad79f

Merge branch 'master' into varconst/port-affecting-batches

ac1a01b

Merge branch 'varconst/port-affecting-batches' of github.com:firebase…

c40e50f

…/firebase-js-sdk into varconst/port-affecting-batches

var-const commented Jul 31, 2018

View reviewed changes

mikelehen approved these changes Jul 31, 2018

View reviewed changes

Merge branch 'master' into varconst/port-affecting-batches

e6cf4f5

var-const commented Jul 31, 2018

View reviewed changes

var-const merged commit ca008a4 into master Jul 31, 2018

wilhuff added a commit that referenced this pull request Jul 31, 2018

Add notes for #1055

373afb7

wilhuff added a commit that referenced this pull request Jul 31, 2018

Add a CHANGELOG entry for #1052 (#1071)

08907db

* Add a CHANGELOG entry for #1052 * Add notes for #1055

schmidt-sebastian mentioned this pull request Aug 1, 2018

Merge #1073

Merged

Salakar mentioned this pull request Aug 2, 2018

Optimistic updates slow? invertase/react-native-firebase#1225

Closed

wilhuff mentioned this pull request Aug 3, 2018

Significantly decreased performance with persistence enabled #513

Closed

schmidt-sebastian mentioned this pull request Aug 3, 2018

Multi-Tab Merge #1000

Merged

wilhuff mentioned this pull request Aug 8, 2018

Add a CHANGELOG entry for #1052 #1071

Merged

firebase locked and limited conversation to collaborators Oct 17, 2019

hsubox76 deleted the varconst/port-affecting-batches branch May 2, 2024 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port optimizations to LocalDocumentsView from iOS #1055

Port optimizations to LocalDocumentsView from iOS #1055

var-const commented Jul 27, 2018

mikelehen left a comment

mikelehen Jul 27, 2018

var-const Jul 29, 2018

mikelehen Jul 27, 2018

var-const Jul 27, 2018

mikelehen Jul 30, 2018

mikelehen Jul 27, 2018

var-const Jul 29, 2018

mikelehen Jul 27, 2018

var-const Jul 27, 2018

mikelehen left a comment

mikelehen Jul 30, 2018

var-const Jul 31, 2018

mikelehen Jul 31, 2018

var-const Jul 31, 2018

mikelehen Jul 30, 2018

var-const left a comment •

edited

Loading

var-const Jul 31, 2018

mikelehen commented Jul 31, 2018

var-const left a comment

var-const Jul 31, 2018

mikelehen left a comment

var-const Jul 31, 2018

Port optimizations to LocalDocumentsView from iOS #1055

Port optimizations to LocalDocumentsView from iOS #1055

Conversation

var-const commented Jul 27, 2018

mikelehen 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

mikelehen 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

var-const left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen commented Jul 31, 2018

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment •

edited

Loading