Don't deserialize full Document proto for Query execution #2115

schmidt-sebastian · 2019-08-26T23:26:39Z

The PR adds a lazy-deserialization option to Documents. This allows Query filter evaluation without parsing the entire document Proto.

This is a port of firebase/firebase-android-sdk#561 with two additions:

Document.field() also caches cache misses.
Document.data is now Document.data() since we perform an expensive conversion.

Stats from Chrome:

10 documents out of a 1000, 10 iterations: Average with PR 317ms, without 356ms.
980 documents out of a 1000, 10 iterations: Average with PR 376ms, without 472ms.

var-const

Can you do some simple benchmarks?

similar to the Android PR, one where only a few documents are returned from a large collection;
one where all or almost all documents are returned from a large collection (to make sure there's no performance regression).

var-const · 2019-08-28T00:57:18Z

packages/firestore/src/model/document.ts

+          if (protoValue.mapValue) {
+            protoValue = protoValue.mapValue.fields[path.get(i)];
+          } else {
+            protoValue = undefined;


I think this loop would be clearer if it used break:

for (let i = 1; i < path.length; ++i) { if (!protoValue || !protoValue.mapValue) { break; } protoValue = protoValue.mapValue.fields[path.get(i)]; }

I take it you don't like my hidden second loop condition. I moved this to a helper function, which allows me to return undefined instead of breaking (which is important since we don't want to use protoValue if it is not of map type).

var-const · 2019-08-28T00:59:47Z

packages/firestore/src/remote/serializer.ts

@@ -718,15 +723,13 @@ export class JsonProtoSerializer {
      const entityChange = change.documentChange!;
      const key = this.fromName(entityChange.document!.name!);
      const version = this.fromVersion(entityChange.document!.updateTime!);
-      const fields = this.fromFields(entityChange.document!.fields || {});


Hmm, so this removes an existing optimization which, IIUC, is for an unrelated use case. What's the context here?

The optimization this comment eludes to is passing in both fields and entityChange.document, specifically the fact that we pass in the Proto since we need it again later. The change in this PR is about trying to always pass the Proto, and constructing fields only as we need it.

var-const · 2019-08-28T01:01:48Z

packages/firestore/src/model/document.ts

-     * serialization). Might be undefined.
-     */
-    readonly proto?: api.Document
+    private objectValue?: ObjectValue,


Is it ever valid to pass both the objectValue and the proto/converter pair? If not, would it be worthwhile to assert that either only objectValue or else both proto and converter are given? (perhaps that could supersede the assertions inside field and data)

It theoretically allows you to memoize both the proto and the objectValue, which helps when we write an object back to persistence. I changed the asserts in this PR to assert in the constructor that proto and converter are set if objectValue is not defined.

var-const · 2019-08-28T01:04:30Z

packages/firestore/src/model/document.ts

+      let fieldValue = this.fieldValueCache.get(canonicalPath);
+
+      if (fieldValue === undefined) {
+        // Instead of deserializing the full Document proto, we only


This function is pretty long now. Perhaps split this part into a helper function?

if (fieldValue === undefined) { return parseFieldValue(path, canonicalPath); }

I moved the code that extract the Proto value from the protobuf into a separate function, which also helps with the control flow of the loop.

schmidt-sebastian

Stats from Chrome:

10 documents out of a 1000, 10 iterations: Average with PR 317ms, without 356ms.
980 documents out of a 1000, 10 iterations: Average with PR 376ms, without 472ms.

schmidt-sebastian · 2019-08-28T15:26:24Z

packages/firestore/src/model/document.ts

+          if (protoValue.mapValue) {
+            protoValue = protoValue.mapValue.fields[path.get(i)];
+          } else {
+            protoValue = undefined;


I take it you don't like my hidden second loop condition. I moved this to a helper function, which allows me to return undefined instead of breaking (which is important since we don't want to use protoValue if it is not of map type).

schmidt-sebastian · 2019-08-28T15:30:17Z

packages/firestore/src/model/document.ts

-     * serialization). Might be undefined.
-     */
-    readonly proto?: api.Document
+    private objectValue?: ObjectValue,


It theoretically allows you to memoize both the proto and the objectValue, which helps when we write an object back to persistence. I changed the asserts in this PR to assert in the constructor that proto and converter are set if objectValue is not defined.

schmidt-sebastian · 2019-08-28T15:30:38Z

packages/firestore/src/model/document.ts

+      let fieldValue = this.fieldValueCache.get(canonicalPath);
+
+      if (fieldValue === undefined) {
+        // Instead of deserializing the full Document proto, we only


I moved the code that extract the Proto value from the protobuf into a separate function, which also helps with the control flow of the loop.

schmidt-sebastian · 2019-08-28T15:34:22Z

packages/firestore/src/remote/serializer.ts

@@ -718,15 +723,13 @@ export class JsonProtoSerializer {
      const entityChange = change.documentChange!;
      const key = this.fromName(entityChange.document!.name!);
      const version = this.fromVersion(entityChange.document!.updateTime!);
-      const fields = this.fromFields(entityChange.document!.fields || {});


The optimization this comment eludes to is passing in both fields and entityChange.document, specifically the fact that we pass in the Proto since we need it again later. The change in this PR is about trying to always pass the Proto, and constructing fields only as we need it.

schmidt-sebastian · 2019-08-28T23:07:21Z

packages/firestore/src/model/document.ts

+      }
+
+      return fieldValue!;
+    }
  }

  fieldValue(path: FieldPath): unknown {


I removed this since it was basically unused. I didn't want to many small function here.

var-const

Thanks for running the numbers! Can you add them to this PR's description?

var-const · 2019-08-29T00:19:32Z

packages/firestore/src/model/document.ts

@@ -180,6 +160,27 @@ export class Document extends MaybeDocument {
    return this.hasLocalMutations || this.hasCommittedMutations;
  }

+  /**
+   * Returns a the nested Protobuf value for 'path`. Can only be called if


Nit: s/a the/the/

var-const · 2019-08-29T00:19:49Z

packages/firestore/src/model/document.ts

+      this.proto !== undefined,
+      "Can only call getProtoField() when proto is defined"
+    );
+    let protoValue: api.Value | undefined = this.proto!.fields[


Nit: add an empty line after assert?

var-const · 2019-08-29T00:20:00Z

packages/firestore/src/model/document.ts

+        return undefined;
+      }
+      protoValue = protoValue.mapValue.fields[path.get(i)];
+    }


Nit: add an empty line right after the loop?

This broke when data became data() in #2115

schmidt-sebastian requested a review from var-const August 26, 2019 23:26

schmidt-sebastian assigned var-const Aug 26, 2019

Don't deserialize full Document proto for Query execution

8a276f2

schmidt-sebastian force-pushed the mrschmidt/dontdeserialize branch from 531f1c1 to 8a276f2 Compare August 27, 2019 03:08

var-const suggested changes Aug 28, 2019

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Aug 28, 2019

schmidt-sebastian commented Aug 28, 2019

View reviewed changes

schmidt-sebastian added 2 commits August 28, 2019 16:12

Address feeback

f318dcd

[AUTOMATED]: Prettier Code Styling

91c228d

schmidt-sebastian force-pushed the mrschmidt/dontdeserialize branch from d6c9871 to f318dcd Compare August 28, 2019 23:13

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Aug 28, 2019

Merge

549aa06

var-const approved these changes Aug 29, 2019

View reviewed changes

schmidt-sebastian added 3 commits August 28, 2019 17:26

Comments

69e402b

[AUTOMATED]: Prettier Code Styling

27cb4fe

Fix test

1ded5b1

schmidt-sebastian merged commit c7e5012 into master Aug 29, 2019

schmidt-sebastian deleted the mrschmidt/dontdeserialize branch August 29, 2019 04:02

schmidt-sebastian added a commit that referenced this pull request Sep 11, 2019

Fixing Document.toString()

26f0156

This broke when data became data() in #2115

schmidt-sebastian mentioned this pull request Sep 11, 2019

Fixing Document.toString() #2168

Merged

schmidt-sebastian added a commit that referenced this pull request Sep 12, 2019

Fixing Document.toString() (#2168)

20e8a07

This broke when data became data() in #2115

david0178418 mentioned this pull request Sep 27, 2019

Error thrown when empty map is present in firestore document #2204

Closed

firebase locked and limited conversation to collaborators Oct 8, 2019

Don't deserialize full Document proto for Query execution #2115

Don't deserialize full Document proto for Query execution #2115

Uh oh!

Conversation

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

Uh oh!

Uh oh!

var-const 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!

schmidt-sebastian 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!

var-const left a comment • 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!

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!

Uh oh!

schmidt-sebastian commented Aug 26, 2019 •

edited

Loading

var-const left a comment •

edited

Loading