From 72f67ede868a02f9a56169b076ba0725a2f15eae Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 11:55:20 -0700 Subject: [PATCH 1/7] Re-enable spec tests --- .../Example/Tests/SpecTests/json/listen_spec_test.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json b/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json index 3a43466fb3c..54144a6b610 100644 --- a/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json @@ -4060,7 +4060,7 @@ "Persists global resume tokens on unlisten": { "describeName": "Listens:", "itName": "Persists global resume tokens on unlisten", - "tags": ["no-ios"], + "tags": [], "config": { "useGarbageCollection": false, "numClients": 1 @@ -4248,7 +4248,7 @@ "Omits global resume tokens for a short while": { "describeName": "Listens:", "itName": "Omits global resume tokens for a short while", - "tags": ["no-ios"], + "tags": [], "config": { "useGarbageCollection": false, "numClients": 1 @@ -4423,7 +4423,7 @@ "Persists global resume tokens if the snapshot is old enough": { "describeName": "Listens:", "itName": "Persists global resume tokens if the snapshot is old enough", - "tags": ["no-ios"], + "tags": [], "config": { "useGarbageCollection": false, "numClients": 1 From dc032913f3d1228604926282b6e60c51b5d1078c Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 11:53:40 -0700 Subject: [PATCH 2/7] Implement the global resume token --- .../Example/Tests/SpecTests/FSTSpecTests.mm | 6 +- Firestore/Source/Local/FSTLocalStore.mm | 67 ++++++++++++++++++- Firestore/Source/Remote/FSTRemoteEvent.mm | 26 ++++++- .../firestore/model/snapshot_version.cc | 13 ++++ .../firestore/model/snapshot_version.h | 3 + 5 files changed, 109 insertions(+), 6 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index f0b7d85573c..2af726a45bf 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -78,6 +78,10 @@ NSString *const kNoLRUTag = @"no-lru"; +static NSString *Describe(NSData *data) { + return [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding]; +} + @interface FSTSpecTests () @property(nonatomic, strong) FSTSyncEngineTestDriver *driver; @@ -660,7 +664,7 @@ - (void)validateActiveTargets { XCTAssertEqualObjects(actual.query, queryData.query); XCTAssertEqual(actual.targetID, queryData.targetID); XCTAssertEqual(actual.snapshotVersion, queryData.snapshotVersion); - XCTAssertEqualObjects(actual.resumeToken, queryData.resumeToken); + XCTAssertEqualObjects(Describe(actual.resumeToken), Describe(queryData.resumeToken)); } [actualTargets removeObjectForKey:targetID]; diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 625a2778d6e..05554af56e1 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -52,6 +52,14 @@ NS_ASSUME_NONNULL_BEGIN +/** + * The maximum time to leave a resume token buffered without writing it out. This value is + * arbitrary: it's long enough to avoid several writes (possibly indefinitely if updates come more + * frequently than this) but short enough that restarting after crashing will still have a pretty + * recent resume token. + */ +static const int64_t kResumeTokenMaxAgeMicros = 5 * 60 * 1000 * 1000; // 5 minutes + @interface FSTLocalStore () /** Manages our in-memory or durable persistence. */ @@ -276,11 +284,15 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent { // than documents that were previously removed from this target. NSData *resumeToken = change.resumeToken; if (resumeToken.length > 0) { + FSTQueryData *oldQueryData = queryData; queryData = [queryData queryDataByReplacingSnapshotVersion:remoteEvent.snapshotVersion resumeToken:resumeToken sequenceNumber:sequenceNumber]; self.targetIDs[boxedTargetID] = queryData; - [self.queryCache updateQueryData:queryData]; + + if ([self shouldPersistQueryData:queryData oldQueryData:oldQueryData change:change]) { + [self.queryCache updateQueryData:queryData]; + } } } @@ -337,6 +349,42 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent { }); } +/** + * Returns YES if the newQueryData should be persisted during an update of an active target. + * QueryData should always be persisted when a target is being released and should not call this + * function. + * + * While the target is active, QueryData updates can be omitted when nothing about the target has + * changed except metadata like the resume token or snapshot version. Occasionally it's worth the + * extra write to prevent these values from getting too stale after a crash, but this doesn't have + * to be too frequent. + */ +- (BOOL)shouldPersistQueryData:(FSTQueryData *)newQueryData + oldQueryData:(FSTQueryData *)oldQueryData + change:(FSTTargetChange *)change { + // Avoid clearing any existing value + if (newQueryData.resumeToken.length == 0) return NO; + + // Any resume token is interesting if there isn't one already. + if (oldQueryData.resumeToken.length == 0) return YES; + + // Don't allow resume token changes to be buffered indefinitely. This allows us to be reasonably + // up-to-date after a crash and avoids needing to loop over all active queries on shutdown. + // Especially in the browser we may not get time to do anything interesting while the current + // tab is closing. + int64_t timeDelta = + newQueryData.snapshotVersion.ToMicroseconds() - oldQueryData.snapshotVersion.ToMicroseconds(); + if (timeDelta >= kResumeTokenMaxAgeMicros) return YES; + + // Otherwise if the only thing that has changed about a target is its resume token it's not + // worth persisting. Note that the RemoteStore keeps an in-memory view of the currently active + // targets which includes the current resume token, so stream failure or user changes will still + // use an up-to-date resume token regardless of what we do here. + size_t changes = change.addedDocuments.size() + change.modifiedDocuments.size() + + change.removedDocuments.size(); + return changes > 0; +} + - (void)notifyLocalViewChanges:(NSArray *)viewChanges { self.persistence.run("NotifyLocalViewChanges", [&]() { FSTReferenceSet *localViewReferences = self.localViewReferences; @@ -390,9 +438,22 @@ - (void)releaseQuery:(FSTQuery *)query { FSTQueryData *queryData = [self.queryCache queryDataForQuery:query]; HARD_ASSERT(queryData, "Tried to release nonexistent query: %s", query); - [self.localViewReferences removeReferencesForID:queryData.targetID]; + FSTTargetID targetID = queryData.targetID; + FSTBoxedTargetID *boxedTargetID = @(targetID); + + FSTQueryData *cachedQueryData = self.targetIDs[boxedTargetID]; + if (cachedQueryData.snapshotVersion > queryData.snapshotVersion) { + // If we've been avoiding persisting the resumeToken (see shouldPersistQueryData for + // conditions and rationale) we need to persist the token now because there will no + // longer be an in-memory version to fall back on. + queryData = cachedQueryData; + [self.queryCache updateQueryData:queryData]; + } + + [self.localViewReferences removeReferencesForID:targetID]; + [self.targetIDs removeObjectForKey:boxedTargetID]; [self.persistence.referenceDelegate removeTarget:queryData]; - [self.targetIDs removeObjectForKey:@(queryData.targetID)]; + [self.targetIDs removeObjectForKey:boxedTargetID]; // If this was the last watch target, then we won't get any more watch snapshots, so we should // release any held batch results. diff --git a/Firestore/Source/Remote/FSTRemoteEvent.mm b/Firestore/Source/Remote/FSTRemoteEvent.mm index 5e5ec1f63d5..9058c58aa70 100644 --- a/Firestore/Source/Remote/FSTRemoteEvent.mm +++ b/Firestore/Source/Remote/FSTRemoteEvent.mm @@ -20,6 +20,7 @@ #include #include #include +#include #import "Firestore/Source/Core/FSTQuery.h" #import "Firestore/Source/Core/FSTViewSnapshot.h" @@ -39,6 +40,7 @@ using firebase::firestore::model::DocumentKeyHash; using firebase::firestore::model::DocumentKeySet; using firebase::firestore::model::SnapshotVersion; +using firebase::firestore::model::TargetId; using firebase::firestore::util::Hash; NS_ASSUME_NONNULL_BEGIN @@ -338,8 +340,7 @@ - (void)handleDocumentChange:(FSTDocumentWatchChange *)documentChange { } - (void)handleTargetChange:(FSTWatchTargetChange *)targetChange { - for (FSTBoxedTargetID *boxedTargetID in targetChange.targetIDs) { - int targetID = boxedTargetID.intValue; + for (TargetId targetID : [self targetIdsForChange:targetChange]) { FSTTargetState *targetState = [self ensureTargetStateForTarget:targetID]; switch (targetChange.state) { case FSTWatchTargetChangeStateNoChange: @@ -387,6 +388,27 @@ - (void)handleTargetChange:(FSTWatchTargetChange *)targetChange { } } +/** + * Returns all targetIds that the watch change applies to: either the targetIds explicitly listed + * in the change or the targetIds of all currently active targets. + */ +- (std::vector)targetIdsForChange:(FSTWatchTargetChange *)targetChange { + NSArray *targetIDs = targetChange.targetIDs; + std::vector result; + if (targetIDs.count > 0) { + result.reserve(targetIDs.count); + for (NSNumber *targetID in targetIDs) { + result.push_back(targetID.intValue); + } + } else { + result.reserve(_targetStates.size()); + for (const auto &entry : _targetStates) { + result.push_back(entry.first); + } + } + return result; +} + - (void)removeTarget:(FSTTargetID)targetID { _targetStates.erase(targetID); } diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc index 114e313f4cc..8b3379ba42f 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc @@ -16,6 +16,8 @@ #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h" +#include // NOLINT(build/c++11) + namespace firebase { namespace firestore { namespace model { @@ -29,6 +31,17 @@ const SnapshotVersion& SnapshotVersion::None() { return kNone; } +int64_t SnapshotVersion::ToMicroseconds() const { + namespace chr = std::chrono; + + auto second_part = + chr::duration_cast(chr::seconds(timestamp_.seconds())); + auto nanosecond_part = chr::duration_cast( + chr::nanoseconds(timestamp_.nanoseconds())); + auto result = second_part + nanosecond_part; + return static_cast(result.count()); +} + } // namespace model } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.h b/Firestore/core/src/firebase/firestore/model/snapshot_version.h index dbecea13c6b..9be75cf35ff 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.h +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.h @@ -43,6 +43,9 @@ class SnapshotVersion { /** Creates a new version that is smaller than all other versions. */ static const SnapshotVersion& None(); + /** Returns a number representation of the version. */ + int64_t ToMicroseconds() const; + #if __OBJC__ size_t Hash() const { return std::hash{}(timestamp_); From ebf744ba243122ffca4d57b8cda49140e41fab7d Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 12:46:07 -0700 Subject: [PATCH 3/7] Update changelog --- Firestore/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index e05ed0becf4..63ea72f9ef1 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,6 +1,8 @@ # Unreleased - [fixed] Fixed an issue where changes to custom authentication claims did not take effect until you did a full sign-out and sign-in. (#1499) +- [changed] Improved how Firestore handles idle queries to reduce the cost of + re-listening within 30 minutes. # v0.13.1 - [fixed] Fixed an issue where `get(source:.Cache)` could throw an From a22ac7b7bbccf6027d7b8c06fe89ab0ddf2e495d Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 15:52:31 -0700 Subject: [PATCH 4/7] Review feedback --- Firestore/Source/Local/FSTLocalStore.mm | 1 - .../src/firebase/firestore/model/snapshot_version.cc | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 05554af56e1..d5836d7649a 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -453,7 +453,6 @@ - (void)releaseQuery:(FSTQuery *)query { [self.localViewReferences removeReferencesForID:targetID]; [self.targetIDs removeObjectForKey:boxedTargetID]; [self.persistence.referenceDelegate removeTarget:queryData]; - [self.targetIDs removeObjectForKey:boxedTargetID]; // If this was the last watch target, then we won't get any more watch snapshots, so we should // release any held batch results. diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc index 8b3379ba42f..50c5e2389a0 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc @@ -34,12 +34,9 @@ const SnapshotVersion& SnapshotVersion::None() { int64_t SnapshotVersion::ToMicroseconds() const { namespace chr = std::chrono; - auto second_part = - chr::duration_cast(chr::seconds(timestamp_.seconds())); - auto nanosecond_part = chr::duration_cast( - chr::nanoseconds(timestamp_.nanoseconds())); - auto result = second_part + nanosecond_part; - return static_cast(result.count()); + auto microseconds = chr::duration_cast( + timestamp_.ToTimePoint().time_since_epoch()); + return static_cast(microseconds.count()); } } // namespace model From 291327faadc3777bf4cde43c389a25caae80cdb6 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 15:57:46 -0700 Subject: [PATCH 5/7] Remove SnapshotVersion.ToMicroseconds --- Firestore/Source/Local/FSTLocalStore.mm | 9 +++++---- .../src/firebase/firestore/model/snapshot_version.cc | 8 -------- .../core/src/firebase/firestore/model/snapshot_version.h | 3 --- 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index d5836d7649a..566308bb70e 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -58,7 +58,7 @@ * frequently than this) but short enough that restarting after crashing will still have a pretty * recent resume token. */ -static const int64_t kResumeTokenMaxAgeMicros = 5 * 60 * 1000 * 1000; // 5 minutes +static const int64_t kResumeTokenMaxAgeSeconds = 5 * 60; // 5 minutes @interface FSTLocalStore () @@ -372,9 +372,10 @@ - (BOOL)shouldPersistQueryData:(FSTQueryData *)newQueryData // up-to-date after a crash and avoids needing to loop over all active queries on shutdown. // Especially in the browser we may not get time to do anything interesting while the current // tab is closing. - int64_t timeDelta = - newQueryData.snapshotVersion.ToMicroseconds() - oldQueryData.snapshotVersion.ToMicroseconds(); - if (timeDelta >= kResumeTokenMaxAgeMicros) return YES; + int64_t newSeconds = newQueryData.snapshotVersion.timestamp().seconds(); + int64_t oldSeconds = oldQueryData.snapshotVersion.timestamp().seconds(); + int64_t timeDelta = newSeconds - oldSeconds; + if (timeDelta >= kResumeTokenMaxAgeSeconds) return YES; // Otherwise if the only thing that has changed about a target is its resume token it's not // worth persisting. Note that the RemoteStore keeps an in-memory view of the currently active diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc index 50c5e2389a0..53e589187c9 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc @@ -31,14 +31,6 @@ const SnapshotVersion& SnapshotVersion::None() { return kNone; } -int64_t SnapshotVersion::ToMicroseconds() const { - namespace chr = std::chrono; - - auto microseconds = chr::duration_cast( - timestamp_.ToTimePoint().time_since_epoch()); - return static_cast(microseconds.count()); -} - } // namespace model } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.h b/Firestore/core/src/firebase/firestore/model/snapshot_version.h index 9be75cf35ff..dbecea13c6b 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.h +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.h @@ -43,9 +43,6 @@ class SnapshotVersion { /** Creates a new version that is smaller than all other versions. */ static const SnapshotVersion& None(); - /** Returns a number representation of the version. */ - int64_t ToMicroseconds() const; - #if __OBJC__ size_t Hash() const { return std::hash{}(timestamp_); From e29d16f5e182a9cea395d9e505d0956522d55a02 Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 16:02:38 -0700 Subject: [PATCH 6/7] fixup --- Firestore/core/src/firebase/firestore/model/snapshot_version.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc index 53e589187c9..114e313f4cc 100644 --- a/Firestore/core/src/firebase/firestore/model/snapshot_version.cc +++ b/Firestore/core/src/firebase/firestore/model/snapshot_version.cc @@ -16,8 +16,6 @@ #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h" -#include // NOLINT(build/c++11) - namespace firebase { namespace firestore { namespace model { From 690733b3da58d9303fd65eff686fec6b3b96d1ce Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Wed, 15 Aug 2018 16:12:21 -0700 Subject: [PATCH 7/7] then --- Firestore/Source/Local/FSTLocalStore.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index 46f795afac8..89bf6e2e5ae 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/Firestore/Source/Local/FSTLocalStore.mm @@ -378,7 +378,7 @@ - (BOOL)shouldPersistQueryData:(FSTQueryData *)newQueryData int64_t timeDelta = newSeconds - oldSeconds; if (timeDelta >= kResumeTokenMaxAgeSeconds) return YES; - // Otherwise if the only thing that has changed about a target is its resume token it's not + // Otherwise if the only thing that has changed about a target is its resume token then it's not // worth persisting. Note that the RemoteStore keeps an in-memory view of the currently active // targets which includes the current resume token, so stream failure or user changes will still // use an up-to-date resume token regardless of what we do here.