Fix RTDB query `getData` caching behavior #9707

jmwski · 2022-04-26T01:18:39Z

This is an attempt at fixing #8286. The root cause is that get doesn't update cache correctly, always calling FSyncTree applyServerOverwriteAtPath, even when getData was called on a query.

There is an alternative method for updating cache called applyTaggedQueryOverwriteAtPath. This method is called in onDataUpdate when the server sends a listener overwrite for a query.

In order to use this method, we need to create an FView for the getData query. I added 4 methods:

FSyncTree registerQuery, unregisterQuery
FSyncPoint registerQuery, unregisterQuery
To maintain the view without adding an event registration.

google-oss-bot · 2022-04-26T01:19:30Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

jmwski · 2022-04-26T01:24:30Z

FirebaseDatabase/Sources/Core/FRepo.m

@@ -533,7 +533,7 @@ - (void)getData:(FIRDatabaseQuery *)query
        }];
        return;
    }
-    [self.persistenceManager setQueryActive:querySpec];
+    NSNumber *tag = [self.serverSyncTree registerQuery:[query querySpec]];


I don't think get queries are tagged on the backend, but it looks like the tag is only used retrieve the query spec in applyTaggedQueryOverwriteAtPath.

jmwski · 2022-04-26T01:25:25Z

FirebaseDatabase/Sources/Core/FSyncPoint.m

-       forNonExistingViewForQuery:(FQuerySpec *)query
-                      writesCache:(FWriteTreeRef *)writesCache
-                      serverCache:(FCacheNode *)serverCache {
+- (void)registerQuery:(FQuerySpec *)query


This method does everything that addEventRegistration did before actually adding an event registration to the view.

google-oss-bot · 2022-04-26T01:28:49Z

Size Report ¹

Affected Products

FirebaseDatabase
Type Base (aefb2a4) Merge (e6e6199) Diff
CocoaPods 1.36 MB 1.36 MB +8 B (+0.0%)

Test Logs

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

jmwski · 2022-04-26T01:36:10Z

FirebaseDatabase/Sources/Core/FSyncPoint.m

@@ -199,6 +205,15 @@ - (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
    return [view initialEvents:eventRegistration];
 }

+- (BOOL)unregisterQuery:(FQuerySpec *)query {


This method just removes the query from the view map if no other query added an event registration to it.

There is quite a bit of logic that isn't brought over from removeEventRegistration. In particular, we are not:

Removing all empty views from a syncpoint if the getData query is default.

Generating any FCancelEvents for the query since there is no event registration associated with it.

I'm least sure about this function.

google-oss-bot · 2022-04-26T01:36:35Z

Coverage Report ¹

Affected Products

FirebaseDatabase-iOS-FirebaseDatabase.framework
Overall coverage changed from 56.89% (aefb2a4) to 56.63% (e6e6199) by -0.27%.
Filename Base (aefb2a4) Merge (e6e6199) Diff
FRepo.m 13.42% 13.25% -0.18%
FSyncPoint.m 86.99% 84.44% -2.56%
FSyncTree.m 70.73% 66.04% -4.69%

Test Logs

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

jmwski · 2022-04-26T01:44:01Z

@jsdt I am hoping you could give me some early feedback on this change. I added a test that checks for the reported issue. More in progress.

FirebaseDatabase/Sources/Core/FRepo.m

jmwski · 2022-04-26T14:00:11Z

FirebaseDatabase/Sources/Core/FSyncTree.m

+        }
+        if (removed) {
+            [self removeTags:@[ query ]];
+            [self.persistenceManager setQueryInactive:query];


Note: only call setQueryInactive when we just removed the last query in the view. This is consistent with how removeEventRegistration works.

jsdt · 2022-04-26T20:31:41Z

FirebaseDatabase/Tests/Integration/FIRDatabaseGetTests.m

+
+@implementation FIRDatabaseGetTests
+
+- (void)testGetDoesntTriggerExtraListens {


Can we also add a test to ensure that a subsequent listen for the same query returns the cached data immediately?

jsdt · 2022-05-04T19:09:24Z

FirebaseDatabase/Sources/Core/FSyncPoint.h

@@ -70,5 +70,9 @@
 - (BOOL)viewExistsForQuery:(FQuerySpec *)query;
 - (BOOL)hasCompleteView;
 - (FView *)completeView;
+- (void)registerQuery:(FQuerySpec *)query


Can you add comments for any new functions?

jsdt · 2022-05-04T19:53:25Z

FirebaseDatabase/Tests/Integration/FIRDatabaseGetTests.m

+#import "FirebaseDatabase/Tests/Helpers/FTestExpectations.h"
+#import "FirebaseDatabase/Tests/Helpers/FTupleEventTypeString.h"
+
+@implementation FIRDatabaseGetTests


Do we have a test where we perform a get, then perform a listen before the get is finished?

jsdt · 2022-05-04T19:56:54Z

FirebaseDatabase/Sources/Core/FSyncTree.m

@@ -448,6 +457,42 @@ - (NSArray *)applyTaggedServerRangeMergeAtPath:(FPath *)path
    }
 }

+- (NSNumber *)registerQuery:(FQuerySpec *)query {


Query is used a lot, including for listeners. Should we call this something like "registerGet"?

maneesht · 2022-05-12T17:15:34Z

FirebaseDatabase/Sources/Core/FSyncTree.m

+                                                   atPath:path];
+    }
+    NSNumber *tag = nil;
+    BOOL viewAlreadyExists = [syncPoint viewExistsForQuery:query];


Seems like the code up until here is reused from addEventRegistration, can we extract this out?

maneesht · 2022-05-12T17:16:17Z

FirebaseDatabase/Sources/Core/FSyncTree.m

+           whileBlock:^BOOL(FPath *pathToSyncPoint, FSyncPoint *syncPoint) {
+             foundAncestorDefaultView =
+                 foundAncestorDefaultView || [syncPoint hasCompleteView];
+             return !foundAncestorDefaultView;


I don't actually think we're using this block, since we aren't checking foundAncestorDefaultView later in the function

Yes, will remove this.

jmwski · 2022-05-12T19:08:17Z

FirebaseDatabase/Sources/Core/FSyncTree.m

+                     writesCache:writesCache
+                     serverCache:serverCache];
+    } else if (![query loadsAllData]) {
+        tag = [self tagQuery:query];


This needs to be moved into the

if (!viewAlreadyExists) { ... }

bock above.

jmwski added 3 commits April 25, 2022 09:45

wip

442a70a

tmp

35257d2

Fix RTDB query get caching behavior

713bda2

google-oss-bot added the api: database label Apr 26, 2022

jmwski commented Apr 26, 2022

View reviewed changes

m

b0dac20

jmwski assigned jsdt Apr 26, 2022

mortenbekditlevsen reviewed Apr 26, 2022

View reviewed changes

FirebaseDatabase/Sources/Core/FRepo.m Show resolved Hide resolved

unregisterQuery

6b04339

jmwski commented Apr 26, 2022

View reviewed changes

jmwski requested a review from maneesht April 26, 2022 17:58

jsdt reviewed May 4, 2022

View reviewed changes

maneesht reviewed May 12, 2022

View reviewed changes

jmwski commented May 12, 2022

View reviewed changes

jmwski closed this Feb 17, 2023

firebase locked and limited conversation to collaborators Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RTDB query `getData` caching behavior #9707

Fix RTDB query `getData` caching behavior #9707

jmwski commented Apr 26, 2022

google-oss-bot commented Apr 26, 2022

jmwski Apr 26, 2022

jmwski Apr 26, 2022

google-oss-bot commented Apr 26, 2022 •

edited

Loading

`FirebaseDatabase`

jmwski Apr 26, 2022 •

edited

Loading

google-oss-bot commented Apr 26, 2022 •

edited

Loading

`FirebaseDatabase-iOS-FirebaseDatabase.framework`

jmwski commented Apr 26, 2022

jmwski Apr 26, 2022

jsdt Apr 26, 2022

jsdt May 4, 2022

jsdt May 4, 2022

jsdt May 4, 2022

maneesht May 12, 2022

maneesht May 12, 2022

jmwski May 12, 2022

jmwski May 12, 2022


		@implementation FIRDatabaseGetTests

		- (void)testGetDoesntTriggerExtraListens {

Fix RTDB query getData caching behavior #9707

Fix RTDB query getData caching behavior #9707

Conversation

jmwski commented Apr 26, 2022

google-oss-bot commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Apr 26, 2022 • edited Loading

Size Report 1

Affected Products

FirebaseDatabase

Test Logs

jmwski Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

google-oss-bot commented Apr 26, 2022 • edited Loading

Coverage Report 1

Affected Products

FirebaseDatabase-iOS-FirebaseDatabase.framework

Test Logs

jmwski commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix RTDB query `getData` caching behavior #9707

Fix RTDB query `getData` caching behavior #9707

google-oss-bot commented Apr 26, 2022 •

edited

Loading

Size Report ¹

`FirebaseDatabase`

jmwski Apr 26, 2022 •

edited

Loading

google-oss-bot commented Apr 26, 2022 •

edited

Loading

Coverage Report ¹

`FirebaseDatabase-iOS-FirebaseDatabase.framework`