Make Documents mutable #4548

schmidt-sebastian · 2021-02-26T17:10:00Z

This is a port of firebase/firebase-android-sdk#2383 and firebase/firebase-android-sdk#2468. The goal is to reduce the number of object creations, so that we can have clear object ownership in the C++ clients. With this change, the documents are owned either by the RemoteDocumentCache or by the View that contains them. Mutations mutate these existing documents and do not create copies.

The existing document hierarchy is removed and replaced by a single MutableDocument type that allows documents to transition between four states:

INVALID, which is a document we only created so we can later apply a mutation on it. This allows documents to be created by SetMutations
FOUND, corresponds to the old Document
NO_DOCUMENT, corresponds to the old NoDocument
UNKNOWN_DOCUMENT, corresponds to the old UnknownDocument

I kept Document, but it is now an interface that makes sure that we no longer modify the documents once we store them in View. MutableDocuments are produces by the RemoteDocumentCache and via RemoteEvents. We then apply all mutations and from then onwards treat them as Documents.

ObjectValue and ObjectValueBuilder are also merged to reduce immutable object creation.

There should be no functional changes. firebase/firebase-android-sdk#2383 contains a bunch of discussions that might help with this review.

changeset-bot · 2021-02-26T17:10:04Z

⚠️ No Changeset found

Latest commit: 723b942

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2021-02-26T20:40:53Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	259 kB	258 kB	-863 B (-0.3%)
esm2017	201 kB	200 kB	-941 B (-0.5%)
main	530 kB	529 kB	-832 B (-0.2%)
module	259 kB	258 kB	-863 B (-0.3%)
react-native	201 kB	201 kB	-941 B (-0.5%)

`@firebase/firestore-exp`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	199 kB	198 kB	-938 B (-0.5%)
main	505 kB	504 kB	-832 B (-0.2%)
module	199 kB	198 kB	-938 B (-0.5%)
react-native	200 kB	199 kB	-937 B (-0.5%)

`@firebase/firestore-lite`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	64.9 kB	65.0 kB	+131 B (+0.2%)
main	143 kB	144 kB	+1.17 kB (+0.8%)
module	64.9 kB	65.0 kB	+131 B (+0.2%)
react-native	65.1 kB	65.2 kB	+131 B (+0.2%)

`@firebase/firestore/bundle`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	266 kB	265 kB	-862 B (-0.3%)
esm2017	155 kB	154 kB	-855 B (-0.6%)
main	526 kB	525 kB	-832 B (-0.2%)
module	266 kB	265 kB	-862 B (-0.3%)
react-native	155 kB	154 kB	-855 B (-0.6%)

`@firebase/firestore/memory`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	196 kB	195 kB	-782 B (-0.4%)
esm2017	152 kB	151 kB	-852 B (-0.6%)
main	324 kB	323 kB	-706 B (-0.2%)
module	196 kB	195 kB	-782 B (-0.4%)
react-native	152 kB	151 kB	-852 B (-0.6%)

`@firebase/firestore/memory-bundle`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
browser	204 kB	204 kB	-787 B (-0.4%)
esm2017	155 kB	154 kB	-855 B (-0.6%)
main	321 kB	320 kB	-706 B (-0.2%)
module	204 kB	204 kB	-787 B (-0.4%)
react-native	155 kB	154 kB	-855 B (-0.6%)

`firebase`

Type	Base (`5019599`)	Head (`ca59ec1`)	Diff
firebase-firestore.js	303 kB	303 kB	-740 B (-0.2%)
firebase-firestore.memory.js	243 kB	242 kB	-687 B (-0.3%)
firebase.js	845 kB	844 kB	-691 B (-0.1%)

Test Logs

Base (50195991): https://github.com/firebase/firebase-js-sdk/actions/runs/604381044
Head (ca59ec1c): https://github.com/firebase/firebase-js-sdk/actions/runs/611795085

schmidt-sebastian · 2021-03-01T18:11:22Z

packages/firestore/src/core/view.ts

-        let newDoc = newMaybeDoc instanceof Document ? newMaybeDoc : null;
-        if (newDoc) {
-          debugAssert(
-            key.isEqual(newDoc.key),
-            'Mismatching keys found in document changes: ' +
-              key +
-              ' != ' +
-              newDoc.key
-          );
-          newDoc = queryMatches(this.query, newDoc) ? newDoc : null;
-        }


This should be the only actual change here.

schmidt-sebastian · 2021-03-01T19:59:18Z

packages/firestore/test/unit/model/object_value_builder.test.ts

-    const builder = new ObjectValueBuilder();
-    const object = builder.build();
-    expect(object.isEqual(ObjectValue.empty())).to.be.true;
+describe('MutableObjectValue', () => {


I would like to rename this file to mutable_object_value.test.ts but GitHub then sees it as a new file, so I will do this after the first review.

FYI I will handle this when I port firebase/firebase-android-sdk#2494

thebrianchen

Mostly documentation nits with a few questions. Thanks for taking the time to go over this with me!

packages/firestore/src/core/bundle_impl.ts

thebrianchen · 2021-03-12T21:10:05Z

packages/firestore/src/exp/reference_impl.ts

@@ -140,7 +139,7 @@ export function getDocFromCache<T>(
        reference._key,
        doc,
        new SnapshotMetadata(
-          doc instanceof Document ? doc.hasLocalMutations : false,
+          doc !== null && doc.hasLocalMutations,


Why don't we have to check the Document's state is FOUND_DOCUMENT?

We only use Document or null in our API layer. That's why I kept this pattern - anything that is not a Document is returned as null once it leaves FirestoreClient.

thebrianchen · 2021-03-13T00:33:28Z

packages/firestore/src/local/local_store_impl.ts

            txn,
            changedDocs
          );
+          return changedDocs;


Do you have to chain changedDocs off of applyLocalViewToDocuments since it's a promise?

Wow. Uhm. Yes. That's a pretty bad bug. Thanks for catching.

thebrianchen · 2021-03-13T00:34:42Z

packages/firestore/src/local/local_store_impl.ts

-              )
-            );
+              );
+              return changedDocs;


Same here as above. Do we have to chain it?

thebrianchen · 2021-03-13T00:35:58Z

packages/firestore/src/local/remote_document_cache.ts

@@ -48,7 +44,7 @@ export interface RemoteDocumentCache {
  getEntry(
    transaction: PersistenceTransaction,
    documentKey: DocumentKey
-  ): PersistencePromise<MaybeDocument | null>;
+  ): PersistencePromise<MutableDocument>;


Update jsdoc since we don't have MaybeDocument anymore. Also update the RemoteDocumentCache interface description as well.

Fixed here and on top.

thebrianchen · 2021-03-15T22:35:52Z

packages/firestore/src/remote/serializer.ts

@@ -441,13 +443,13 @@ function fromMissing(
  );
  const key = fromName(serializer, result.missing);
  const version = fromVersion(result.readTime);
-  return new NoDocument(key, version);
+  return MutableDocument.newNoDocument(key, version);
 }

 export function fromMaybeDocument(


update function name since we don't use maybeDocument anymore.

Updated to fromBatchGetDocumentsResponse

thebrianchen · 2021-03-15T22:41:44Z

packages/firestore/src/remote/watch_change.ts

        this.addDocumentToTarget(targetId, docChange.newDoc);
-      } else if (docChange.newDoc instanceof NoDocument) {
+      } else {


Question: How are we sure that this has type NO_DOCUMENT in order to remove the else if?

This is the Watch code. Watch can either send as a document or a delete. Unknown documents are only used with latency compensation, and invalid documents are only used for document lookups for keys that are not cached.

thebrianchen · 2021-03-15T23:21:06Z

packages/firestore/test/util/helpers.ts

@@ -708,7 +713,7 @@ export function documentSet(...args: unknown[]): DocumentSet {
  }
  for (const doc of args) {
    debugAssert(
-      doc instanceof Document,
+      doc instanceof MutableDocument,


optional: It looks a bit weird that this checks for MutableDocument, but the input parameter is Document[]. docSet.add() also takes in Document as a parameter. Change parameter signature to MutableDocument, or assertion to Document?

Interfaces can't be used in instanceof checks.

thebrianchen · 2021-03-16T14:40:50Z

packages/firestore/test/util/helpers.ts

  for (const docOrKey of docsOrKeys) {
-    if (docOrKey instanceof Document) {
+    if (docOrKey instanceof MutableDocument) {


optional: Same question as comment below, should this use Document instead?

Interfaces can't be used in instanceof checks.

thebrianchen · 2021-03-16T14:54:48Z

packages/firestore/src/model/object_value.ts

+    return this.partialValue;
+  }
+
+  private extractNestedValue(


optional: this method can be static.

Done (FWIW I just got the same comment on the iOS PR)

schmidt-sebastian

Thanks for the thorough review! Someone deserves a vacation!

thebrianchen

lgtm!

thebrianchen · 2021-03-17T16:48:27Z

packages/firestore/src/lite/reference_impl.ts

@@ -134,7 +133,7 @@ export function getDoc<T>(
        reference.firestore,
        userDataWriter,
        reference._key,
-        maybeDocument instanceof Document ? maybeDocument : null,
+        maybeDocument.isFoundDocument() ? maybeDocument : null,


nit: rename maybeDocument variable since that concept no longer exists.

thebrianchen · 2021-03-17T16:51:08Z

packages/firestore/src/local/local_serializer.ts

@@ -63,6 +58,7 @@ import {
  DbUnknownDocument
 } from './indexeddb_schema';
 import { TargetData, TargetPurpose } from './target_data';
+import { Timestamp } from '../exp/timestamp';


I think this was already fixed.

thebrianchen · 2021-03-17T16:56:29Z

packages/firestore/src/model/document.ts

-export interface DocumentOptions {
-  hasLocalMutations?: boolean;
-  hasCommittedMutations?: boolean;
+const enum DocumentType {


optional: add jsdoc description for `DocumentType

thebrianchen · 2021-03-17T16:59:17Z

packages/firestore/src/model/mutation.ts

- * returned.
+ * Applies this mutation to the given document for the purposes of computing a
+ * new remote document. If the input document doesn't match the expected state
+ * (e.g. it is invalid or outdated), the document state may transition to


nit: s/state/type

schmidt-sebastian force-pushed the mrschmidt/mutabledocuments branch from 3993f15 to abda8e9 Compare February 26, 2021 20:13

schmidt-sebastian force-pushed the mrschmidt/mutabledocuments branch from 306e59a to e83e967 Compare February 27, 2021 03:14

Make Documents mutable

ead9034

schmidt-sebastian force-pushed the mrschmidt/mutabledocuments branch from 10587ca to ead9034 Compare March 1, 2021 17:51

schmidt-sebastian changed the title ~~Mrschmidt/mutabledocuments~~ Make Documents mutable Mar 1, 2021

schmidt-sebastian marked this pull request as ready for review March 1, 2021 17:57

schmidt-sebastian commented Mar 1, 2021

View reviewed changes

Simplify

77a52fb

schmidt-sebastian commented Mar 1, 2021

View reviewed changes

Simplify

3c0b3ee

schmidt-sebastian requested a review from thebrianchen March 1, 2021 20:04

schmidt-sebastian assigned thebrianchen Mar 1, 2021

schmidt-sebastian added the api: firestore label Mar 1, 2021

Lint

91ce3e2

firebase deleted a comment from google-oss-bot Mar 1, 2021

firebase deleted a comment from google-oss-bot Mar 2, 2021

thebrianchen reviewed Mar 16, 2021

View reviewed changes

schmidt-sebastian added 5 commits March 16, 2021 14:56

First round

0c18d31

Remove preconditions

9bcba07

Merge

8ea377b

Fix build

ebf7fbf

Remove asserts again

84d6e49

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners March 16, 2021 21:39

Remove docKey

f271609

schmidt-sebastian commented Mar 16, 2021

View reviewed changes

Lint

67e658d

thebrianchen approved these changes Mar 17, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Mar 17, 2021

schmidt-sebastian added 5 commits March 17, 2021 11:37

Final edits

2bd9d1c

Merge branch 'master' into mrschmidt/mutabledocuments

b5faba1

undo API review

723b942

Merge branch 'master' into mrschmidt/mutabledocuments

f4877bd

Merge branch 'master' into mrschmidt/mutabledocuments

a631ce6

firebase deleted a comment from google-oss-bot Mar 20, 2021

schmidt-sebastian merged commit 8c56c25 into master Mar 20, 2021

schmidt-sebastian deleted the mrschmidt/mutabledocuments branch March 20, 2021 01:22

firebase locked and limited conversation to collaborators Apr 20, 2021

Make Documents mutable #4548

Make Documents mutable #4548

Uh oh!

Conversation

schmidt-sebastian commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

google-oss-bot commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore-exp

@firebase/firestore-lite

@firebase/firestore/bundle

@firebase/firestore/memory

@firebase/firestore/memory-bundle

firebase

Test Logs

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!

thebrianchen left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

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

schmidt-sebastian commented Feb 26, 2021 •

edited

Loading

changeset-bot bot commented Feb 26, 2021 •

edited

Loading

google-oss-bot commented Feb 26, 2021 •

edited

Loading

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`firebase`