Remote document reads elision #3246

wu-hui · 2021-12-15T19:43:09Z

No description provided.

google-oss-bot · 2021-12-15T19:57:55Z

Coverage Report ¹

Affected Products

firebase-firestore
Overall coverage changed from 45.25% (5657f12) to 45.26% (af9449d) by +0.02%.
Filename Base (5657f12) Merge (af9449d) Diff
DeleteMutation.java 90.00% 95.00% +5.00%
PatchMutation.java 98.39% 100.00% +1.61%

Test Logs

Notes

Commit (af9449d) is created by Prow via merging PR base commit (5657f12) and head commit (6f97357).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/EJQTWXmIPZ.html

google-oss-bot · 2021-12-15T19:58:17Z

Size Report ¹

Affected Products

firebase-firestore
Type Base (5657f12) Merge (af9449d) Diff
aar 1.23 MB 1.23 MB +49 B (+0.0%)
apk (release) 3.33 MB 3.33 MB -48 B (-0.0%)

Test Logs

Notes

Commit (af9449d) is created by Prow via merging PR base commit (5657f12) and head commit (6f97357).

https://storage.googleapis.com/firebase-sdk-metric-reports/G4Ce2oBnai.html

schmidt-sebastian

Thank you for doing this. Some suggestions to trim this down a bit.

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java

schmidt-sebastian · 2021-12-16T17:11:41Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java

+    }
+
+    Map<DocumentKey, MutableDocument> remoteDocuments =
+        remoteDocumentCache.getAll(query.getPath(), offset, ignore);


I don't think this part of the optimization is quite worth it. We are still reading the elements from the cache. What do you think about not touching getAll()?

It might be useful when user goes offline long enough and there are a lot documents with overlays. It is hard to say at what point it is worth it, but it does not hurt performance so let's keep it?

I'd rather keep it as is. The old way is shorter, easier to understand and does not require porting. The only thing you are saving here is the Proto decoding, which already happens in the background.

OK, removed.

schmidt-sebastian · 2021-12-16T17:17:02Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/InternalDocument.java

+package com.google.firebase.firestore.model;
+
+/** Interface exposing underlying document information that is not visible in {@code Document}. */
+public interface InternalDocument extends Document {


I don't quite understand the need for this indirection. Can you explain your reasoning?

I guess I wanted a bit type safety, that getVersion and getReadTime can only be accessed via this interface, instead of setting them to 0. And DocumentSnapshot holds a Document so no access to version/readtime to user code.

I feel safer this way, comparing to reviewing all places where they are used and reason whether 0 makes sense there. Setting them to 0 also felt "wrong" because we know they are at least the versions they are set now, and setting to 0 gives up that information.

My concern here is the same as with the other change: It adds a layer of indirection which adds porting work and code size. We already use SnapshotVersion.NONE as the "non-existing" snapshot version in a lot of places in our codebase.

If you want to achieve what you are after, you could make the missing versions nullable - but I really thing we should just continue to use SnapshotVersion.NONE.

I also do not quite understand where the name "Internal" comes from. I think you mean something like "FoundDocument", which we used to have but moved away from.

schmidt-sebastian · 2021-12-16T17:17:31Z

...e-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

@@ -164,15 +166,18 @@ public MutableDocument get(DocumentKey documentKey) {
    if (collections.isEmpty()) {
      return Collections.emptyMap();
    } else if (BINDS_PER_STATEMENT * collections.size() < SQLitePersistence.MAX_ARGS) {
-      return getAll(collections, offset, limit);
+      return getAll(collections, offset, limit, new HashSet<>());


Collections.emptySet() to avoid creating a new object.

schmidt-sebastian · 2021-12-16T17:18:11Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/Document.java

-   * Returns the version of this document if it exists or a version at which this document was
-   * guaranteed to not exist.
-   */
-  SnapshotVersion getVersion();


I am not sure where to put this as a comment, but I think we should just set the "version" and "readTime" to 0 in setHasLocalMutations()

See my other comment.

schmidt-sebastian · 2021-12-16T17:19:16Z

firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

@@ -389,7 +389,8 @@ public void testHandlesSetMutationThenDocument() {
            asList(targetId),
            emptyList()));
    assertChanged(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
-    assertContains(doc("foo/bar", 2, map("foo", "bar")).setHasLocalMutations());
+    // Version is 0 because of remote document elision


I don't think we need these comments once we use SnapshotVersion.NONE for all documents with local mutations (plus, I am not sure that the terminology used in this comment makes it obvious what is going on here)

This IS a bit awkward, but let's decide which way to go first.

If we keep the comment, we should phrase it differently and remove the term "elision", which requires too much knowledge to understand.

Removed comments.

…/local/LocalDocumentsView.java Co-authored-by: Sebastian Schmidt <[email protected]>

wu-hui · 2022-01-07T17:58:50Z

/retest

schmidt-sebastian · 2022-01-10T17:13:00Z

firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

@@ -546,9 +548,10 @@ public void testHandlesPatchMutationThenDocumentThenAck() {
    int targetId = allocateQuery(query);
    applyRemoteEvent(
        addedRemoteEvent(
-            doc("foo/bar", 1, map("it", "base")).setHasLocalMutations(),
+            asList(doc("foo/bar", 1, map("it", "base")).setHasLocalMutations()),


I think you can undo the change to "addedRemoteEvent". A remote doc should never have local mutations, so some of these tests are wrong.

wu-hui added 4 commits December 14, 2021 14:48

Initial change, breaks version check

6f1536d

Works for single document reads

0f1d689

Fixing missed test

0f3d956

Code complete

06ab3e2

google-oss-bot added the size/L label Dec 15, 2021

wu-hui added 3 commits December 15, 2021 14:59

License header

35936d0

Reformat

6d1c927

Little re-arrangement.

72c8406

wu-hui requested a review from schmidt-sebastian December 16, 2021 15:35

wu-hui assigned schmidt-sebastian Dec 16, 2021

schmidt-sebastian reviewed Dec 16, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Dec 16, 2021

wu-hui and others added 2 commits January 4, 2022 09:38

Update firebase-firestore/src/main/java/com/google/firebase/firestore…

5eed2a8

…/local/LocalDocumentsView.java Co-authored-by: Sebastian Schmidt <[email protected]>

Feedback.

b48e6b4

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jan 4, 2022

schmidt-sebastian assigned wu-hui and unassigned schmidt-sebastian Jan 4, 2022

Remove getAll change and use None for local mutations

e7143c0

google-oss-bot added size/M and removed size/L labels Jan 7, 2022

wu-hui added 2 commits January 7, 2022 11:40

Test tweaks

9766646

Add comment back.

e86c069

wu-hui assigned schmidt-sebastian and unassigned wu-hui Jan 7, 2022

schmidt-sebastian reviewed Jan 10, 2022

View reviewed changes

schmidt-sebastian approved these changes Jan 10, 2022

View reviewed changes

schmidt-sebastian removed their assignment Jan 10, 2022

wu-hui added 2 commits January 10, 2022 12:31

Merge branch 'master' into wuandy/JustEnoughDocReads

1300f27

Fix merge error

6f97357

wu-hui merged commit 712e5b8 into master Jan 11, 2022

wu-hui deleted the wuandy/JustEnoughDocReads branch January 11, 2022 18:19

firebase locked and limited conversation to collaborators Feb 11, 2022

Remote document reads elision #3246

Remote document reads elision #3246

Uh oh!

Conversation

wu-hui commented Dec 15, 2021

Uh oh!

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

Uh oh!

Coverage Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

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

Uh oh!

Size Report 1

Affected Products

firebase-firestore

Test Logs

Notes

Uh oh!

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

wu-hui commented Jan 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

google-oss-bot commented Dec 15, 2021 •

edited

Loading

Coverage Report ¹

`firebase-firestore`

google-oss-bot commented Dec 15, 2021 •

edited

Loading

Size Report ¹

`firebase-firestore`