Skip to content

Commit faad116

Browse files
var-constvar-const
authored
C++ migration: eliminate FSTBoxedTargetId (#2301)
1 parent 2ca62dd commit faad116

16 files changed

+87
-70
lines changed

‎Firestore/Example/Tests/Integration/FSTDatastoreTests.mm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ - (void)rejectFailedWriteWithBatchID:(BatchId)batchID error:(NSError *)error {
130130
HARD_FAIL("Not implemented");
131131
}
132132

133-
- (DocumentKeySet)remoteKeysForTarget:(FSTBoxedTargetID *)targetId {
133+
- (DocumentKeySet)remoteKeysForTarget:(TargetId)targetId {
134134
return DocumentKeySet{};
135135
}
136136

‎Firestore/Example/Tests/Local/FSTLRUGarbageCollectorTests.mm

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#import <XCTest/XCTest.h>
2020

21+
#include <unordered_map>
2122
#include <unordered_set>
2223

2324
#import "FIRTimestamp.h"
@@ -130,7 +131,8 @@ - (int)queryCountForPercentile:(int)percentile {
130131
}
131132

132133
- (int)removeQueriesThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber
133-
liveQueries:(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries {
134+
liveQueries:(const std::unordered_map<TargetId, FSTQueryData *> &)
135+
liveQueries {
134136
return _persistence.run("gc", [&]() -> int {
135137
return [_gc removeQueriesUpThroughSequenceNumber:sequenceNumber liveQueries:liveQueries];
136138
});
@@ -378,12 +380,12 @@ - (void)testRemoveQueriesUpThroughSequenceNumber {
378380
if ([self isTestBaseClass]) return;
379381

380382
[self newTestResources];
381-
NSMutableDictionary<NSNumber *, FSTQueryData *> *liveQueries = [[NSMutableDictionary alloc] init];
383+
std::unordered_map<TargetId, FSTQueryData *> liveQueries;
382384
for (int i = 0; i < 100; i++) {
383385
FSTQueryData *queryData = [self addNextQuery];
384386
// Mark odd queries as live so we can test filtering out live queries.
385387
if (queryData.targetID % 2 == 1) {
386-
liveQueries[@(queryData.targetID)] = queryData;
388+
liveQueries[queryData.targetID] = queryData;
387389
}
388390
}
389391
// GC up through 20th query, which is 20%.
@@ -639,8 +641,7 @@ - (void)testRemoveTargetsThenGC {
639641
});
640642

641643
// Finally, do the garbage collection, up to but not including the removal of middleTarget
642-
NSDictionary<NSNumber *, FSTQueryData *> *liveQueries =
643-
@{@(oldestTarget.targetID) : oldestTarget};
644+
std::unordered_map<TargetId, FSTQueryData *> liveQueries{{oldestTarget.targetID, oldestTarget}};
644645

645646
int queriesRemoved = [self removeQueriesThroughSequenceNumber:upperBound liveQueries:liveQueries];
646647
XCTAssertEqual(1, queriesRemoved, @"Expected to remove newest target");
@@ -699,7 +700,7 @@ - (void)testDisabled {
699700
});
700701

701702
LruResults results =
702-
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; });
703+
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:{}]; });
703704
XCTAssertFalse(results.didRun);
704705

705706
[_persistence shutdown];
@@ -725,7 +726,7 @@ - (void)testCacheTooSmall {
725726

726727
// Try collection and verify that it didn't run
727728
LruResults results =
728-
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; });
729+
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:{}]; });
729730
XCTAssertFalse(results.didRun);
730731

731732
[_persistence shutdown];
@@ -754,7 +755,7 @@ - (void)testGCRan {
754755

755756
// Mark nothing as live, so everything is eligible.
756757
LruResults results =
757-
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:@{}]; });
758+
_persistence.run("GC", [&]() -> LruResults { return [_gc collectWithLiveTargets:{}]; });
758759

759760
// By default, we collect 10% of the sequence numbers. Since we added 100 targets,
760761
// that should be 10 targets with 10 documents each, for a total of 100 documents.

‎Firestore/Source/Core/FSTSyncEngine.mm

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -154,10 +154,6 @@ @interface FSTSyncEngine ()
154154
@property(nonatomic, strong, readonly)
155155
NSMutableDictionary<FSTQuery *, FSTQueryView *> *queryViewsByQuery;
156156

157-
/** FSTQueryViews for all active queries, indexed by target ID. */
158-
@property(nonatomic, strong, readonly)
159-
NSMutableDictionary<NSNumber *, FSTQueryView *> *queryViewsByTarget;
160-
161157
@end
162158

163159
@implementation FSTSyncEngine {
@@ -168,6 +164,9 @@ @implementation FSTSyncEngine {
168164
std::unordered_map<User, NSMutableDictionary<NSNumber *, FSTVoidErrorBlock> *, HashUser>
169165
_mutationCompletionBlocks;
170166

167+
/** FSTQueryViews for all active queries, indexed by target ID. */
168+
std::unordered_map<TargetId, FSTQueryView *> _queryViewsByTarget;
169+
171170
/**
172171
* When a document is in limbo, we create a special listen to resolve it. This maps the
173172
* DocumentKey of each limbo document to the TargetId of the listen resolving it.
@@ -194,7 +193,6 @@ - (instancetype)initWithLocalStore:(FSTLocalStore *)localStore
194193
_remoteStore = remoteStore;
195194

196195
_queryViewsByQuery = [NSMutableDictionary dictionary];
197-
_queryViewsByTarget = [NSMutableDictionary dictionary];
198196

199197
_targetIdGenerator = TargetIdGenerator::SyncEngineTargetIdGenerator();
200198
_currentUser = initialUser;
@@ -230,7 +228,7 @@ - (FSTViewSnapshot *)initializeViewAndComputeSnapshotForQueryData:(FSTQueryData
230228
resumeToken:queryData.resumeToken
231229
view:view];
232230
self.queryViewsByQuery[queryData.query] = queryView;
233-
self.queryViewsByTarget[@(queryData.targetID)] = queryView;
231+
_queryViewsByTarget[queryData.targetID] = queryView;
234232

235233
return viewChange.snapshot;
236234
}
@@ -401,8 +399,9 @@ - (void)rejectListenWithTargetID:(const TargetId)targetID error:(NSError *)error
401399
limboDocuments:std::move(limboDocuments)];
402400
[self applyRemoteEvent:event];
403401
} else {
404-
FSTQueryView *queryView = self.queryViewsByTarget[@(targetID)];
405-
HARD_ASSERT(queryView, "Unknown targetId: %s", targetID);
402+
auto found = _queryViewsByTarget.find(targetID);
403+
HARD_ASSERT(found != _queryViewsByTarget.end(), "Unknown targetId: %s", targetID);
404+
FSTQueryView *queryView = found->second;
406405
FSTQuery *query = queryView.query;
407406
[self.localStore releaseQuery:query];
408407
[self removeAndCleanupQuery:queryView];
@@ -466,7 +465,7 @@ - (void)assertDelegateExistsForSelector:(SEL)methodSelector {
466465

467466
- (void)removeAndCleanupQuery:(FSTQueryView *)queryView {
468467
[self.queryViewsByQuery removeObjectForKey:queryView.query];
469-
[self.queryViewsByTarget removeObjectForKey:@(queryView.targetID)];
468+
_queryViewsByTarget.erase(queryView.targetID);
470469

471470
DocumentKeySet limboKeys = _limboDocumentRefs.ReferencedKeys(queryView.targetID);
472471
_limboDocumentRefs.RemoveReferences(queryView.targetID);
@@ -599,12 +598,13 @@ - (void)credentialDidChangeWithUser:(const firebase::firestore::auth::User &)use
599598
[self.remoteStore credentialDidChange];
600599
}
601600

602-
- (firebase::firestore::model::DocumentKeySet)remoteKeysForTarget:(FSTBoxedTargetID *)targetId {
603-
const auto iter = _limboResolutionsByTarget.find([targetId intValue]);
601+
- (DocumentKeySet)remoteKeysForTarget:(TargetId)targetId {
602+
const auto iter = _limboResolutionsByTarget.find(targetId);
604603
if (iter != _limboResolutionsByTarget.end() && iter->second.document_received) {
605604
return DocumentKeySet{iter->second.key};
606605
} else {
607-
FSTQueryView *queryView = self.queryViewsByTarget[targetId];
606+
auto found = _queryViewsByTarget.find(targetId);
607+
FSTQueryView *queryView = found != _queryViewsByTarget.end() ? found->second : nil;
608608
return queryView ? queryView.view.syncedDocuments : DocumentKeySet{};
609609
}
610610
}

‎Firestore/Source/Local/FSTLRUGarbageCollector.h

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import <Foundation/Foundation.h>
1818

1919
#include <string>
20+
#include <unordered_map>
2021

2122
#import "FIRFirestoreSettings.h"
2223
#import "Firestore/Source/Local/FSTQueryData.h"
@@ -101,7 +102,9 @@ struct LruResults {
101102
*/
102103
- (int)removeTargetsThroughSequenceNumber:
103104
(firebase::firestore::model::ListenSequenceNumber)sequenceNumber
104-
liveQueries:(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries;
105+
liveQueries:
106+
(const std::unordered_map<firebase::firestore::model::TargetId,
107+
FSTQueryData *> &)liveQueries;
105108

106109
- (size_t)byteSize;
107110

@@ -144,7 +147,9 @@ struct LruResults {
144147
*/
145148
- (int)removeQueriesUpThroughSequenceNumber:
146149
(firebase::firestore::model::ListenSequenceNumber)sequenceNumber
147-
liveQueries:(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries;
150+
liveQueries:
151+
(const std::unordered_map<firebase::firestore::model::TargetId,
152+
FSTQueryData *> &)liveQueries;
148153

149154
/**
150155
* Removes all unreferenced documents from the cache that have a sequence number less than or equal
@@ -156,6 +161,6 @@ struct LruResults {
156161
- (size_t)byteSize;
157162

158163
- (firebase::firestore::local::LruResults)collectWithLiveTargets:
159-
(NSDictionary<NSNumber *, FSTQueryData *> *)liveTargets;
164+
(const std::unordered_map<firebase::firestore::model::TargetId, FSTQueryData *> &)liveTargets;
160165

161166
@end

‎Firestore/Source/Local/FSTLRUGarbageCollector.mm

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
using firebase::firestore::local::LruResults;
3333
using firebase::firestore::model::DocumentKey;
3434
using firebase::firestore::model::ListenSequenceNumber;
35+
using firebase::firestore::model::TargetId;
3536

3637
const int64_t kFIRFirestoreCacheSizeUnlimited = LruParams::CacheSizeUnlimited;
3738
const ListenSequenceNumber kFSTListenSequenceNumberInvalid = -1;
@@ -93,7 +94,8 @@ - (instancetype)initWithDelegate:(id<FSTLRUDelegate>)delegate params:(LruParams)
9394
return self;
9495
}
9596

96-
- (LruResults)collectWithLiveTargets:(NSDictionary<NSNumber *, FSTQueryData *> *)liveTargets {
97+
- (LruResults)collectWithLiveTargets:
98+
(const std::unordered_map<TargetId, FSTQueryData *> &)liveTargets {
9799
if (_params.minBytesThreshold == kFIRFirestoreCacheSizeUnlimited) {
98100
LOG_DEBUG("Garbage collection skipped; disabled");
99101
return LruResults::DidNotRun();
@@ -111,7 +113,8 @@ - (LruResults)collectWithLiveTargets:(NSDictionary<NSNumber *, FSTQueryData *> *
111113
}
112114
}
113115

114-
- (LruResults)runGCWithLiveTargets:(NSDictionary<NSNumber *, FSTQueryData *> *)liveTargets {
116+
- (LruResults)runGCWithLiveTargets:
117+
(const std::unordered_map<TargetId, FSTQueryData *> &)liveTargets {
115118
Timestamp start = Timestamp::Now();
116119
int sequenceNumbers = [self queryCountForPercentile:_params.percentileToCollect];
117120
// Cap at the configured max
@@ -170,8 +173,8 @@ - (ListenSequenceNumber)sequenceNumberForQueryCount:(NSUInteger)queryCount {
170173
}
171174

172175
- (int)removeQueriesUpThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber
173-
liveQueries:
174-
(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries {
176+
liveQueries:(const std::unordered_map<TargetId, FSTQueryData *> &)
177+
liveQueries {
175178
return [_delegate removeTargetsThroughSequenceNumber:sequenceNumber liveQueries:liveQueries];
176179
}
177180

‎Firestore/Source/Local/FSTLevelDB.mm

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "Firestore/Source/Local/FSTLevelDB.h"
1818

1919
#include <memory>
20+
#include <unordered_map>
2021
#include <utility>
2122

2223
#import "FIRFirestoreErrors.h"
@@ -39,6 +40,7 @@
3940
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
4041
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
4142
#include "Firestore/core/src/firebase/firestore/model/resource_path.h"
43+
#include "Firestore/core/src/firebase/firestore/model/types.h"
4244
#include "Firestore/core/src/firebase/firestore/util/filesystem.h"
4345
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
4446
#include "Firestore/core/src/firebase/firestore/util/ordered_code.h"
@@ -71,6 +73,7 @@
7173
using firebase::firestore::model::DocumentKey;
7274
using firebase::firestore::model::ListenSequenceNumber;
7375
using firebase::firestore::model::ResourcePath;
76+
using firebase::firestore::model::TargetId;
7477
using firebase::firestore::util::OrderedCode;
7578
using firebase::firestore::util::Path;
7679
using firebase::firestore::util::Status;
@@ -227,7 +230,8 @@ - (void)removeSentinel:(const DocumentKey &)key {
227230
}
228231

229232
- (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber
230-
liveQueries:(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries {
233+
liveQueries:(const std::unordered_map<TargetId, FSTQueryData *> &)
234+
liveQueries {
231235
return _db.queryCache->RemoveTargets(sequenceNumber, liveQueries);
232236
}
233237

‎Firestore/Source/Local/FSTLocalStore.mm

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#import "Firestore/Source/Local/FSTLocalStore.h"
1818

1919
#include <set>
20+
#include <unordered_map>
2021
#include <utility>
2122

2223
#import "FIRTimestamp.h"
@@ -84,9 +85,6 @@ @interface FSTLocalStore ()
8485
/** Maps a query to the data about that query. */
8586
@property(nonatomic) QueryCache *queryCache;
8687

87-
/** Maps a targetID to data about its query. */
88-
@property(nonatomic, strong) NSMutableDictionary<NSNumber *, FSTQueryData *> *targetIDs;
89-
9088
@end
9189

9290
@implementation FSTLocalStore {
@@ -98,6 +96,9 @@ @implementation FSTLocalStore {
9896

9997
/** The set of document references maintained by any local views. */
10098
ReferenceSet _localViewReferences;
99+
100+
/** Maps a targetID to data about its query. */
101+
std::unordered_map<TargetId, FSTQueryData *> _targetIDs;
101102
}
102103

103104
- (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
@@ -111,8 +112,6 @@ - (instancetype)initWithPersistence:(id<FSTPersistence>)persistence
111112
mutationQueue:_mutationQueue];
112113
[_persistence.referenceDelegate addInMemoryPins:&_localViewReferences];
113114

114-
_targetIDs = [NSMutableDictionary dictionary];
115-
116115
_targetIDGenerator = TargetIdGenerator::QueryCacheTargetIdGenerator(0);
117116
}
118117
return self;
@@ -217,14 +216,14 @@ - (MaybeDocumentMap)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
217216
DocumentKeySet authoritativeUpdates;
218217
for (const auto &entry : remoteEvent.targetChanges) {
219218
TargetId targetID = entry.first;
220-
FSTBoxedTargetID *boxedTargetID = @(targetID);
221219
FSTTargetChange *change = entry.second;
222220

223221
// Do not ref/unref unassigned targetIDs - it may lead to leaks.
224-
FSTQueryData *queryData = self.targetIDs[boxedTargetID];
225-
if (!queryData) {
222+
auto found = _targetIDs.find(targetID);
223+
if (found == _targetIDs.end()) {
226224
continue;
227225
}
226+
FSTQueryData *queryData = found->second;
228227

229228
// When a global snapshot contains updates (either add or modify) we can completely trust
230229
// these updates as authoritative and blindly apply them to our cache (as a defensive measure
@@ -253,7 +252,7 @@ - (MaybeDocumentMap)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
253252
queryData = [queryData queryDataByReplacingSnapshotVersion:remoteEvent.snapshotVersion
254253
resumeToken:resumeToken
255254
sequenceNumber:sequenceNumber];
256-
self.targetIDs[boxedTargetID] = queryData;
255+
_targetIDs[targetID] = queryData;
257256

258257
if ([self shouldPersistQueryData:queryData oldQueryData:oldQueryData change:change]) {
259258
_queryCache->UpdateTarget(queryData);
@@ -394,10 +393,10 @@ - (FSTQueryData *)allocateQuery:(FSTQuery *)query {
394393
return cached;
395394
});
396395
// Sanity check to ensure that even when resuming a query it's not currently active.
397-
FSTBoxedTargetID *boxedTargetID = @(queryData.targetID);
398-
HARD_ASSERT(!self.targetIDs[boxedTargetID], "Tried to allocate an already allocated query: %s",
399-
query);
400-
self.targetIDs[boxedTargetID] = queryData;
396+
TargetId targetID = queryData.targetID;
397+
HARD_ASSERT(_targetIDs.find(targetID) == _targetIDs.end(),
398+
"Tried to allocate an already allocated query: %s", query);
399+
_targetIDs[targetID] = queryData;
401400
return queryData;
402401
}
403402

@@ -407,9 +406,9 @@ - (void)releaseQuery:(FSTQuery *)query {
407406
HARD_ASSERT(queryData, "Tried to release nonexistent query: %s", query);
408407

409408
TargetId targetID = queryData.targetID;
410-
FSTBoxedTargetID *boxedTargetID = @(targetID);
411409

412-
FSTQueryData *cachedQueryData = self.targetIDs[boxedTargetID];
410+
auto found = _targetIDs.find(targetID);
411+
FSTQueryData *cachedQueryData = found != _targetIDs.end() ? found->second : nil;
413412
if (cachedQueryData.snapshotVersion > queryData.snapshotVersion) {
414413
// If we've been avoiding persisting the resumeToken (see shouldPersistQueryData for
415414
// conditions and rationale) we need to persist the token now because there will no
@@ -426,7 +425,7 @@ - (void)releaseQuery:(FSTQuery *)query {
426425
for (const DocumentKey &key : removed) {
427426
[self.persistence.referenceDelegate removeReference:key];
428427
}
429-
[self.targetIDs removeObjectForKey:boxedTargetID];
428+
_targetIDs.erase(targetID);
430429
[self.persistence.referenceDelegate removeTarget:queryData];
431430
});
432431
}

‎Firestore/Source/Local/FSTMemoryPersistence.mm

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
using firebase::firestore::model::DocumentKey;
4242
using firebase::firestore::model::DocumentKeyHash;
4343
using firebase::firestore::model::ListenSequenceNumber;
44+
using firebase::firestore::model::TargetId;
4445
using firebase::firestore::util::Status;
4546

4647
using MutationQueues = std::unordered_map<User, FSTMemoryMutationQueue *, HashUser>;
@@ -235,7 +236,8 @@ - (void)enumerateMutationsUsingBlock:
235236
}
236237

237238
- (int)removeTargetsThroughSequenceNumber:(ListenSequenceNumber)sequenceNumber
238-
liveQueries:(NSDictionary<NSNumber *, FSTQueryData *> *)liveQueries {
239+
liveQueries:(const std::unordered_map<TargetId, FSTQueryData *> &)
240+
liveQueries {
239241
return _persistence.queryCache->RemoveTargets(sequenceNumber, liveQueries);
240242
}
241243

‎Firestore/Source/Remote/FSTRemoteStore.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ NS_ASSUME_NONNULL_BEGIN
8080
* Returns the set of remote document keys for the given target ID. This list includes the
8181
* documents that were assigned to the target when we received the last snapshot.
8282
*/
83-
- (firebase::firestore::model::DocumentKeySet)remoteKeysForTarget:(FSTBoxedTargetID *)targetId;
83+
- (firebase::firestore::model::DocumentKeySet)remoteKeysForTarget:
84+
(firebase::firestore::model::TargetId)targetId;
8485

8586
@end
8687

0 commit comments

Comments
 (0)