Update query-document mapping from bundles. #3620

wu-hui · 2020-08-12T20:03:34Z

No description provided.

changeset-bot · 2020-08-12T20:03:37Z

💥 No Changeset

Latest commit: 1d5a4a6

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, 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 · 2020-08-12T20:22:37Z

Binary Size Report

Affected SDKs

@firebase/analytics

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 9.89 kB ? (?)

main ? 11.1 kB ? (?)

module ? 10.8 kB ? (?)

`@firebase/app`

Type	Base (`15b96a9`)	Head (`e52e401`)	Diff
browser	?	11.1 kB	? (?)
esm2017	?	9.46 kB	? (?)
lite	?	9.11 kB	? (?)
lite-esm2017	?	7.75 kB	? (?)
main	?	10.1 kB	? (?)
module	?	11.0 kB	? (?)
react-native	?	9.87 kB	? (?)

@firebase/auth

Type Base (15b96a9) Head (e52e401) Diff

browser ? 177 kB ? (?)

main ? 177 kB ? (?)

module ? 177 kB ? (?)
@firebase/component

Type Base (15b96a9) Head (e52e401) Diff

browser ? 5.16 kB ? (?)

esm2017 ? 3.90 kB ? (?)

main ? 5.16 kB ? (?)

module ? 5.05 kB ? (?)
@firebase/database

Type Base (15b96a9) Head (e52e401) Diff

browser ? 269 kB ? (?)

esm2017 ? 236 kB ? (?)

main ? 270 kB ? (?)

module ? 268 kB ? (?)
@firebase/firestore

Type Base (15b96a9) Head (e52e401) Diff

browser ? 261 kB ? (?)

esm2017 ? 204 kB ? (?)

main ? 497 kB ? (?)

module ? 258 kB ? (?)

react-native ? 204 kB ? (?)
@firebase/firestore/exp

Type Base (15b96a9) Head (e52e401) Diff

browser ? 197 kB ? (?)

main ? 490 kB ? (?)

module ? 197 kB ? (?)

react-native ? 197 kB ? (?)
@firebase/firestore/lite

Type Base (15b96a9) Head (e52e401) Diff

browser ? 64.7 kB ? (?)

main ? 143 kB ? (?)

module ? 64.7 kB ? (?)

react-native ? 64.8 kB ? (?)
@firebase/firestore/memory

Type Base (15b96a9) Head (e52e401) Diff

browser ? 200 kB ? (?)

esm2017 ? 155 kB ? (?)

main ? 371 kB ? (?)

module ? 197 kB ? (?)

react-native ? 156 kB ? (?)
@firebase/functions

Type Base (15b96a9) Head (e52e401) Diff

browser ? 9.64 kB ? (?)

esm2017 ? 7.30 kB ? (?)

main ? 9.67 kB ? (?)

module ? 9.45 kB ? (?)
@firebase/installations

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 16.5 kB ? (?)

main ? 22.0 kB ? (?)

module ? 21.5 kB ? (?)
@firebase/logger

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 3.25 kB ? (?)

main ? 5.14 kB ? (?)

module ? 4.83 kB ? (?)
@firebase/messaging

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 23.2 kB ? (?)

main ? 31.4 kB ? (?)

module ? 30.9 kB ? (?)
@firebase/performance

Type Base (15b96a9) Head (e52e401) Diff

browser ? 27.1 kB ? (?)

esm2017 ? 25.1 kB ? (?)

main ? 27.1 kB ? (?)

module ? 26.8 kB ? (?)
@firebase/polyfill

Type Base (15b96a9) Head (e52e401) Diff

main ? 775 B ? (?)

module ? 705 B ? (?)
@firebase/remote-config

Type Base (15b96a9) Head (e52e401) Diff

browser ? 23.1 kB ? (?)

esm2017 ? 17.7 kB ? (?)

main ? 23.1 kB ? (?)

module ? 22.7 kB ? (?)
@firebase/storage

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 54.9 kB ? (?)

main ? 61.3 kB ? (?)

module ? 61.1 kB ? (?)
@firebase/testing

Type Base (15b96a9) Head (e52e401) Diff

main ? 6.34 kB ? (?)
@firebase/util

Type Base (15b96a9) Head (e52e401) Diff

browser ? 20.5 kB ? (?)

esm2017 ? 18.3 kB ? (?)

main ? 20.5 kB ? (?)

module ? 19.5 kB ? (?)
@firebase/webchannel-wrapper

Type Base (15b96a9) Head (e52e401) Diff

esm2017 ? 39.4 kB ? (?)

main ? 41.0 kB ? (?)

module ? 40.6 kB ? (?)

`firebase`

Click to show 15 binary size changes.

Type	Base (`15b96a9`)	Head (`e52e401`)	Diff
firebase-analytics.js	?	28.3 kB	? (?)
firebase-app.js	?	20.0 kB	? (?)
firebase-auth.js	?	173 kB	? (?)
firebase-database.js	?	187 kB	? (?)
firebase-firestore.js	?	299 kB	? (?)
firebase-firestore.memory.js	?	239 kB	? (?)
firebase-functions.js	?	9.88 kB	? (?)
firebase-installations.js	?	19.2 kB	? (?)
firebase-messaging.js	?	39.2 kB	? (?)
firebase-performance-standalone.es2017.js	?	72.1 kB	? (?)
firebase-performance-standalone.js	?	47.4 kB	? (?)
firebase-performance.js	?	37.8 kB	? (?)
firebase-remote-config.js	?	37.0 kB	? (?)
firebase-storage.js	?	39.9 kB	? (?)
firebase.js	?	833 kB	? (?)

Test Logs

Head (e52e4017): https://github.com/firebase/firebase-js-sdk/actions/runs/208820930

schmidt-sebastian · 2020-08-12T20:25:14Z

packages/firestore/src/protos/firestore_bundle_proto.ts

@@ -54,6 +54,9 @@ export interface BundledDocumentMetadata {

  /** BundledDocumentMetadata exists */
  exists?: boolean | null;
+
+  /** The names of the queries in this bundle that this document matches to. */
+  queries?: string[] | null;


In Proto3, is not possible to have a missing array. Missing arrays are represented as empty lists, so this should be queries: string[].

schmidt-sebastian · 2020-08-12T20:27:49Z

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

@@ -1706,6 +1758,69 @@ function genericLocalStoreTests(
      .finish();
  });

+  it('handles loading named queries allocates targets and updates target document mapping', async () => {


Suggested change

it('handles loading named queries allocates targets and updates target document mapping', async () => {

it('loading named queries allocates targets and updates target document mapping', async () => {

schmidt-sebastian · 2020-08-12T20:29:57Z

packages/firestore/src/core/bundle.ts

+    const result = await applyBundleDocuments(this.localStore, this.documents);
+


As far as I can tell, returning the map of matching documents from applyBundleDocuments saves one iteration. This code would be less complex if you extracted the mapping in this function (ideally, via a call to a helper function). This would remove your changes from applyBundleDocuments and some of the changes from LocalStoreTester.

Done. It IS nicer this way!

schmidt-sebastian · 2020-08-12T20:30:48Z

packages/firestore/src/local/local_store.ts

  const localStoreImpl = debugCast(localStore, LocalStoreImpl);
  const bundleConverter = new BundleConverter(localStoreImpl.serializer);
  let documentMap = maybeDocumentMap();
  let versionMap = documentVersionMap();
+  const queryDocumentMap: Map<string, DocumentKeySet> = new Map();


Suggested change

const queryDocumentMap: Map<string, DocumentKeySet> = new Map();

const queryDocumentMap = new Map<string, DocumentKeySet>();

schmidt-sebastian · 2020-08-12T20:36:27Z

packages/firestore/src/local/local_store.ts

+          localStoreImpl.bundleCache.saveNamedQuery(transaction, query)
+        )
+        .next(() =>
+          localStoreImpl.targetCache.addMatchingKeys(


I think you should only do this if you updated the readTime above. On top of that, you need to also remove all existing keys, otherwise your mapping will not match the backend's mapping.

Agree to both.

schmidt-sebastian · 2020-08-13T16:24:27Z

packages/firestore/src/core/bundle.ts

+    const queryDocumentMap = getQueryDocumentMapping(
+      this.localStore,
+      this.documents
+    );


Nit: Empty line below.

schmidt-sebastian · 2020-08-13T16:26:11Z

packages/firestore/src/local/local_store.ts

+): Map<string, DocumentKeySet> {
+  const localStoreImpl = debugCast(localStore, LocalStoreImpl);
+  const queryDocumentMap = new Map<string, DocumentKeySet>();
+  const bundleConverter = new BundleConverter(localStoreImpl.serializer);


Is it possible to get the serializer from the callsite of this function, drop its dependency on "LocalStore" and move it to bundle.ts?

I had to add a getSerializer to localstore interface, but that still seems better than current.

schmidt-sebastian · 2020-08-13T16:31:17Z

packages/firestore/src/local/local_store.ts

+        .next(changedDocuments => {
+          return PersistencePromise.resolve(changedDocuments);


This looks like a no-op.

schmidt-sebastian · 2020-08-13T16:31:46Z

packages/firestore/src/local/local_store.ts

@@ -1389,21 +1417,41 @@ export async function saveNamedQuery(
    'readwrite',
    transaction => {
      // Update allocated target's read time, if the bundle's read time is newer.
-      let updateReadTime = PersistencePromise.resolve();
+      let updateReadTime = PersistencePromise.resolve(false);


If still needed: s/updateReadTime/updatedReadTime (or readTimeUpdated)

schmidt-sebastian · 2020-08-13T16:33:15Z

packages/firestore/src/local/local_store.ts

+            return localStoreImpl.targetCache
+              .removeMatchingKeysForTargetId(transaction, allocated.targetId)
+              .next(() =>
+                localStoreImpl.targetCache.addMatchingKeys(
+                  transaction,
+                  documents,
+                  allocated.targetId
+                )
+              );


Could this not just be chained off the call to "updateTargetData"?

schmidt-sebastian · 2020-08-13T16:33:56Z

packages/firestore/src/local/local_store.ts

+          .next(
+            () => true,
+            () => false
+          );


This needs explanation (but I think it could be removed if you update the target mapping right here)

schmidt-sebastian

Can you incorporate the following cleanup: https://gist.github.com/schmidt-sebastian/d51c9c9ec7be723fc8f208d69c4bbd9f (it probably needs some fixing)

It passes in the serializer instead of obtaining it from LocalStore, which improves encapsulation
It simplifies some of your transaction logic

wu-hui · 2020-08-14T15:03:59Z

Applied with some minor fixes.

schmidt-sebastian · 2020-08-14T15:34:09Z

packages/firestore/exp/src/api/database.ts

-  );
+  getSyncEngine(firestoreImpl).then(async syncEngine => {
+    enqueueLoadBundle(
+      (await firestoreImpl._getConfiguration()).databaseInfo.databaseId,


This style of code is pretty hard to debug (a lot of the magic happens inline). Do you mind extracting a local variable for this?

Update query-document mapping from bundles.

7d808c1

wu-hui requested a review from schmidt-sebastian August 12, 2020 20:05

wu-hui assigned schmidt-sebastian Aug 12, 2020

schmidt-sebastian reviewed Aug 12, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 12, 2020

Only update mapping when query read time is updated.

d62f9cc

wu-hui assigned schmidt-sebastian and unassigned wu-hui Aug 13, 2020

Fix lint errors

b00494e

schmidt-sebastian reviewed Aug 13, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Aug 13, 2020

Better code structure.

e2f17ca

wu-hui assigned schmidt-sebastian and unassigned schmidt-sebastian and wu-hui Aug 14, 2020

schmidt-sebastian reviewed Aug 14, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui Aug 14, 2020

Use serializer constructed from firestore client.

322e27d

wu-hui assigned schmidt-sebastian and unassigned schmidt-sebastian and wu-hui Aug 14, 2020

schmidt-sebastian approved these changes Aug 14, 2020

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Aug 14, 2020

Don't inline await

1d5a4a6

wu-hui merged commit b4f9936 into wuandy/Bundles Aug 14, 2020

firebase locked and limited conversation to collaborators Sep 14, 2020

	it('handles loading named queries allocates targets and updates target document mapping', async () => {
	it('loading named queries allocates targets and updates target document mapping', async () => {

		const result = await applyBundleDocuments(this.localStore, this.documents);

	const queryDocumentMap: Map<string, DocumentKeySet> = new Map();
	const queryDocumentMap = new Map<string, DocumentKeySet>();

		.next(changedDocuments => {
		return PersistencePromise.resolve(changedDocuments);

Update query-document mapping from bundles. #3620

Update query-document mapping from bundles. #3620

Uh oh!

Conversation

wu-hui commented Aug 12, 2020

Uh oh!

changeset-bot bot commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 No Changeset

Uh oh!

google-oss-bot commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/analytics

@firebase/app

@firebase/auth

@firebase/component

@firebase/database

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/firestore/memory

@firebase/functions

@firebase/installations

@firebase/logger

@firebase/messaging

@firebase/performance

@firebase/polyfill

@firebase/remote-config

@firebase/storage

@firebase/testing

@firebase/util

@firebase/webchannel-wrapper

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!

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!

Choose a reason for hiding this comment

changeset-bot bot commented Aug 12, 2020 •

edited

Loading

google-oss-bot commented Aug 12, 2020 •

edited

Loading

`@firebase/analytics`

`@firebase/app`

`@firebase/auth`

`@firebase/component`

`@firebase/database`

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`@firebase/functions`

`@firebase/installations`

`@firebase/logger`

`@firebase/messaging`

`@firebase/performance`

`@firebase/polyfill`

`@firebase/remote-config`

`@firebase/storage`

`@firebase/testing`

`@firebase/util`

`@firebase/webchannel-wrapper`

`firebase`