Skip to content

Multi-Tab Merge #1000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 171 commits into from
Aug 3, 2018
Merged

Multi-Tab Merge #1000

merged 171 commits into from
Aug 3, 2018

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Jul 11, 2018

This PR merges the Multi-Tab changes into Master. The Multi-Tab feature itself is not turned on, and cannot be turned on by end users yet (the flag to do so is commented out).

@schmidt-sebastian
Copy link
Contributor Author

I am planning to squash this into Master (as merges seemed to be turned off these days). If you think we should merge this instead, then I will go ahead and clean up some of the earlier commit history in this PR that contains individual commits.

@mikelehen
Copy link
Contributor

Hrm... This is going to be difficult to review in a meaningful way. :-) Any chance you can help by trying to provide some sort of risk assessment? Given the flag is disabled, what code paths are most affected and should be re-reviewed for sanity? What perf impacts have you measured or expect there to be? Or are you confident this will have very little impact?

@schmidt-sebastian
Copy link
Contributor Author

Hrm... This is going to be difficult to review in a meaningful way. :-) Any chance you can help by trying to provide some sort of risk assessment? Given the flag is disabled, what code paths are most affected and should be re-reviewed for sanity? What perf impacts have you measured or expect there to be? Or are you confident this will have very little impact?

I looked through the changes in here to spot bad merges (without success so I consider this a success).

The performance changes (and the code that can't turned off) all live in the persistence layer.

  • Mutation require an extra read on insert.
  • Query targets require an extra read on insert.
  • The keys of changes documents are written to the documentChangesStore as we receive remote events.

I have tested the first two scenarios and will get back to you when I tested the third one.

Reading of mutations and raising of query snapshot should be unaffected and I was not able to spot a performance difference.

@schmidt-sebastian
Copy link
Contributor Author

schmidt-sebastian commented Jul 26, 2018

Note that we have much improved performance numbers now thanks to 
https://github.com/firebase/firebase-js-sdk/pull/1055,
https://github.com/firebase/firebase-js-sdk/pull/1065, https://github.com/firebase/firebase-js-sdk/pull/1063
and https://github.com/firebase/firebase-js-sdk/pull/1051

* 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)
@schmidt-sebastian
Copy link
Contributor Author

Merged master :) Please note the changes in 76e0e1e#diff-cf8f425b1bd34112162e05d44fc3a464

Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YOLO

@schmidt-sebastian schmidt-sebastian merged commit 2f17ba0 into master Aug 3, 2018
@schmidt-sebastian schmidt-sebastian deleted the firestore-multi-tab branch August 3, 2018 21:21
@firebase firebase locked and limited conversation to collaborators Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants