In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go #1505

var-const · 2018-07-09T16:28:33Z

This uses the newly-added allMutationBatchesAffectingDocumentKeys to find/deserialize all such batches in one go and then reuse the batches while processing a set of keys.

var-const · 2018-07-09T19:08:09Z

Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm

+// truncated), or -1 if the OS call fails.
+// TODO(varconst): move the helper function and the test into a new test target for performance
+// testing.
+long long getCurrentMemoryUsedInMb() {


Note: there is also something like performance tests in XCTest. I originally had that here as well, but eventually dropped it for this case at least. It seems like a very limited and frankly a clunky feature. It's only possible to measure execution speed, the test is run 10 times which I think is impossible to configure, and to top it all, expected time can only be set in the UI (it is then stored in a file similar to a scheme).

wilhuff

Basically LGTM with nits

wilhuff · 2018-07-09T18:12:44Z

Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm

+// truncated), or -1 if the OS call fails.
+// TODO(varconst): move the helper function and the test into a new test target for performance
+// testing.
+long long getCurrentMemoryUsedInMb() {


We should avoid long long. Probably this can be int64_t.

wilhuff · 2018-07-09T20:24:48Z

Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm

+    XCTAssertNotEqual(memoryUsedAfterCommitMb, -1);
+    const long long memoryDeltaMb = memoryUsedAfterCommitMb - memoryUsedBeforeCommitMb;
+    // This by its nature cannot be a precise value. A regression would be on the scale of 500Mb.
+    XCTAssertLessThan(memoryDeltaMb, 150);


Is this the same for both 32 and 64-bit builds?

I tried it using two simulators in debug mode:

iPhone 8 Plus (64 bit) -- I see increases around 90 MB;

iPhone 4S with iOS 9.3 (presumably 32bit) -- I see increases around 50 MB.

I don't have a real device handy, but I can try to find one if you'd like.

wilhuff · 2018-07-09T20:25:38Z

Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm

+// truncated), or -1 if the OS call fails.
+// TODO(varconst): move the helper function and the test into a new test target for performance
+// testing.
+long long getCurrentMemoryUsedInMb() {


Also, free functions should start with an initial capital.

wilhuff · 2018-07-09T20:28:15Z

Firestore/Source/Local/FSTLocalDocumentsView.mm

+
+// Internal version of documentForKey: which allows reusing `affectingBatches`.
+- (nullable FSTMaybeDocument *)documentForKey:(const DocumentKey &)key
+                         withAffectingBatches:(NSArray<FSTMutationBatch *> *)affectingBatches {


In general avoid prepositions in secondary arguments unless their meaning is required to understand the behavior. A good usage would be copyFromFile:toFile:. This doesn't need "with".

Also, "affecting" really is part of the function that looks up the mutations in the mutation queue, but after that point they're just batches. Taken together I'd suggest renaming this to documentForKey:inBatches:.

Similarly I'd suggest renaming locals of the form affectingBatches to just batches or matchingBatches.

Also, "affecting" really is part of the function that looks up the mutations in the mutation queue, but after that point they're just batches.

My understanding is that these are supposed to be batches relevant to whatever local view the function is trying to build (as opposed to, say, all pending). So I thought it helpful to somehow reflect that in the name. However, I don't feel strongly about it; if you think just batches is clear enough, I'm fine with it. (renamed local variable to batches and parameter name to inBatches throughout the delta).

wilhuff · 2018-07-09T23:04:20Z

The lint failure here is spurious. Please add mach to _C_SYSTEM_DIRECTORIES in cpplint.py:

https://github.com/firebase/firebase-ios-sdk/blob/master/scripts/cpplint.py#L417

Also if you have a better way of distinguishing these from other headers that doesn't rely on whitelisting I'll happily take it in this PR too :-).

wilhuff · 2018-07-09T23:06:21Z

Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm

+  XCTAssertNotEqual(memoryUsedBeforeCommitMb, -1);
+  [batch commitWithCompletion:^(NSError *_Nullable error) {
+    XCTAssertNil(error);
+    const std::int64_t memoryUsedAfterCommitMb = GetCurrentMemoryUsedInMb();


Elsewhere we just call this type int64_t. The extra decoration is just noise.

Done.

I think we were previously using that because we were using <stdint.h>, but I don't feel too strongly about it.

…#1534) Reflects #1505, #1507, #1533.

* 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).

* 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)

wilhuff and others added 3 commits June 28, 2018 17:55

Pod updates for Cocapods 1.5.3

c6c2a9c

Add allMutationsAffectingDocumentKeys

76bf96a

Initial

36d8596

var-const added the api: firestore label Jul 9, 2018

var-const assigned wilhuff Jul 9, 2018

var-const requested a review from wilhuff July 9, 2018 16:28

googlebot added the cla: yes label Jul 9, 2018

var-const commented Jul 9, 2018

View reviewed changes

var-const mentioned this pull request Jul 9, 2018

Eliminate unnecessary DocumentKey->FSTDocumentKey conversions #1507

Merged

wilhuff approved these changes Jul 9, 2018

View reviewed changes

wilhuff reviewed Jul 9, 2018

View reviewed changes

wilhuff assigned var-const and unassigned wilhuff Jul 9, 2018

var-const added 2 commits July 9, 2018 18:36

Review feedback

b70157c

style.sh

19d862a

var-const changed the title ~~In FSTLocalDocumentsView, search for all batches affecting a key in one go~~ In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go Jul 9, 2018

var-const added 2 commits July 9, 2018 18:38

undo change to project file

8283a7b

Merge branch 'master' into varconst/write-batch-opt-1

7f0c933

This comment has been minimized.

Sign in to view

googlebot added cla: no and removed cla: yes labels Jul 9, 2018

var-const changed the base branch from wilhuff/leveldb-mutations to master July 9, 2018 22:39

This comment has been minimized.

Sign in to view

googlebot added cla: yes and removed cla: no labels Jul 9, 2018

Merge branch 'master' into varconst/write-batch-opt-1

06dad9b

var-const force-pushed the varconst/write-batch-opt-1 branch from 946614b to 06dad9b Compare July 9, 2018 22:49

var-const assigned wilhuff and unassigned var-const and wilhuff Jul 9, 2018

var-const assigned var-const and wilhuff and unassigned var-const Jul 9, 2018

Add <mach> to system directories

7b366a1

wilhuff reviewed Jul 9, 2018

View reviewed changes

Review feedback

e29283f

var-const merged commit ff95ffc into master Jul 10, 2018

var-const deleted the varconst/write-batch-opt-1 branch July 10, 2018 17:59

var-const mentioned this pull request Jul 14, 2018

Update changelog to mention performance improvements in write batches #1534

Merged

var-const added a commit that referenced this pull request Jul 17, 2018

Update changelog to mention performance improvements in write batches (…

d6d273a

…#1534) Reflects #1505, #1507, #1533.

var-const mentioned this pull request Jul 27, 2018

Port optimizations to LocalDocumentsView from iOS firebase/firebase-js-sdk#1055

Merged

firebase locked and limited conversation to collaborators Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go #1505

In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go #1505

Uh oh!

var-const commented Jul 9, 2018

Uh oh!

var-const Jul 9, 2018

Uh oh!

wilhuff left a comment

Uh oh!

wilhuff Jul 9, 2018

Uh oh!

var-const Jul 9, 2018

Uh oh!

wilhuff Jul 9, 2018

Uh oh!

var-const Jul 9, 2018

Uh oh!

wilhuff Jul 9, 2018

Uh oh!

var-const Jul 9, 2018

Uh oh!

wilhuff Jul 9, 2018

Uh oh!

var-const Jul 9, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

wilhuff commented Jul 9, 2018

Uh oh!

wilhuff Jul 9, 2018

Uh oh!

var-const Jul 10, 2018

Uh oh!

Uh oh!

In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go #1505

In FSTLocalDocumentsView, search for all batches affecting a set of keys in one go #1505

Uh oh!

Conversation

var-const commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff 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!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

wilhuff commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!