Skip to content

Commit

Permalink
Keep track of number of queries in the query cache (firebase#776)
Browse files Browse the repository at this point in the history
* Implement schema versions

* Style fixes

* newlines, copyrights, assumptions

* Fix nullability

* Raw ptr -> shared_ptr

* kVersionTableGlobal -> kVersionGlobalTable

* Drop utils, move into static methods

* Drop extra include

* Add a few more comments

* Move version constant into migrations file

* formatting?

* Fix comment

* Split add and update queryData

* Work on adding targetCount

* More work on count

* Using shared_ptr

* Implement count for query cache

* use quotes

* Add cast

* Styling

* Revert year bump in copyright

* Add adversarial key to migration test

* Add comment

* Fix style
  • Loading branch information
Greg Soltis committed Feb 13, 2018
1 parent c7c51a7 commit 5f5f808
Show file tree
Hide file tree
Showing 11 changed files with 159 additions and 24 deletions.
29 changes: 29 additions & 0 deletions Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,18 @@
#include <leveldb/db.h>

#import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
#import "Firestore/Source/Local/FSTLevelDBKey.h"
#import "Firestore/Source/Local/FSTLevelDBMigrations.h"
#import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
#import "Firestore/Source/Local/FSTWriteGroup.h"

#include "Firestore/core/src/firebase/firestore/util/ordered_code.h"

#import "Firestore/Example/Tests/Local/FSTPersistenceTestHelpers.h"

NS_ASSUME_NONNULL_BEGIN

using firebase::firestore::util::OrderedCode;
using leveldb::DB;
using leveldb::Options;
using leveldb::Status;
Expand Down Expand Up @@ -70,6 +75,30 @@ - (void)testSetsVersionNumber {
XCTAssertGreaterThan(actual, 0, @"Expected to migrate to a schema version > 0");
}

- (void)testCountsQueries {
NSUInteger expected = 50;
FSTWriteGroup *group = [FSTWriteGroup groupWithAction:@"Setup"];
for (int i = 0; i < expected; i++) {
std::string key = [FSTLevelDBTargetKey keyWithTargetID:i];
[group setData:"dummy" forKey:key];
}
// Add a dummy entry after the targets to make sure the iteration is correctly bounded.
// Use a table that would sort logically right after that table 'target'.
std::string dummyKey;
// Magic number that indicates a table name follows. Needed to mimic the prefix to the target
// table.
OrderedCode::WriteSignedNumIncreasing(&dummyKey, 5);
OrderedCode::WriteString(&dummyKey, "targetA");
[group setData:"dummy" forKey:dummyKey];

Status status = [group writeToDB:_db];
XCTAssertTrue(status.ok(), @"Failed to write targets");

[FSTLevelDBMigrations runMigrationsOnDB:_db];
FSTPBTargetGlobal *metadata = [FSTLevelDBQueryCache readTargetMetadataFromDB:_db];
XCTAssertEqual(expected, metadata.targetCount, @"Failed to count all of the targets we added");
}

@end

NS_ASSUME_NONNULL_END
3 changes: 3 additions & 0 deletions Firestore/Example/Tests/Local/FSTQueryCacheTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,20 @@ - (void)testCanonicalIDCollision {

FSTQueryData *data2 = [self queryDataWithQuery:q2];
[self addQueryData:data2];
XCTAssertEqual(2, [self.queryCache count]);

XCTAssertEqualObjects([self.queryCache queryDataForQuery:q1], data1);
XCTAssertEqualObjects([self.queryCache queryDataForQuery:q2], data2);

[self removeQueryData:data1];
XCTAssertNil([self.queryCache queryDataForQuery:q1]);
XCTAssertEqualObjects([self.queryCache queryDataForQuery:q2], data2);
XCTAssertEqual(1, [self.queryCache count]);

[self removeQueryData:data2];
XCTAssertNil([self.queryCache queryDataForQuery:q1]);
XCTAssertNil([self.queryCache queryDataForQuery:q2]);
XCTAssertEqual(0, [self.queryCache count]);
}

- (void)testSetQueryToNewValue {
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Protos/FrameworkMaker.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@
);
inputPaths = (
"${SRCROOT}/Pods/Target Support Files/Pods-FrameworkMaker_iOS/Pods-FrameworkMaker_iOS-resources.sh",
"$PODS_CONFIGURATION_BUILD_DIR/gRPC/gRPCCertificates.bundle",
"${PODS_CONFIGURATION_BUILD_DIR}/gRPC/gRPCCertificates.bundle",
);
name = "[CP] Copy Pods Resources";
outputPaths = (
Expand Down
4 changes: 4 additions & 0 deletions Firestore/Protos/objc/firestore/local/Target.pbobjc.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ typedef GPB_ENUM(FSTPBTargetGlobal_FieldNumber) {
FSTPBTargetGlobal_FieldNumber_HighestTargetId = 1,
FSTPBTargetGlobal_FieldNumber_HighestListenSequenceNumber = 2,
FSTPBTargetGlobal_FieldNumber_LastRemoteSnapshotVersion = 3,
FSTPBTargetGlobal_FieldNumber_TargetCount = 4,
};

/**
Expand Down Expand Up @@ -197,6 +198,9 @@ typedef GPB_ENUM(FSTPBTargetGlobal_FieldNumber) {
/** Test to see if @c lastRemoteSnapshotVersion has been set. */
@property(nonatomic, readwrite) BOOL hasLastRemoteSnapshotVersion;

/** On platforms that need it, holds the number of targets persisted. */
@property(nonatomic, readwrite) int32_t targetCount;

@end

NS_ASSUME_NONNULL_END
Expand Down
11 changes: 11 additions & 0 deletions Firestore/Protos/objc/firestore/local/Target.pbobjc.m
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,12 @@ @implementation FSTPBTargetGlobal
@dynamic highestTargetId;
@dynamic highestListenSequenceNumber;
@dynamic hasLastRemoteSnapshotVersion, lastRemoteSnapshotVersion;
@dynamic targetCount;

typedef struct FSTPBTargetGlobal__storage_ {
uint32_t _has_storage_[1];
int32_t highestTargetId;
int32_t targetCount;
GPBTimestamp *lastRemoteSnapshotVersion;
int64_t highestListenSequenceNumber;
} FSTPBTargetGlobal__storage_;
Expand Down Expand Up @@ -224,6 +226,15 @@ + (GPBDescriptor *)descriptor {
.flags = GPBFieldOptional,
.dataType = GPBDataTypeMessage,
},
{
.name = "targetCount",
.dataTypeSpecific.className = NULL,
.number = FSTPBTargetGlobal_FieldNumber_TargetCount,
.hasIndex = 3,
.offset = (uint32_t)offsetof(FSTPBTargetGlobal__storage_, targetCount),
.flags = GPBFieldOptional,
.dataType = GPBDataTypeInt32,
},
};
GPBDescriptor *localDescriptor =
[GPBDescriptor allocDescriptorForClass:[FSTPBTargetGlobal class]
Expand Down
3 changes: 3 additions & 0 deletions Firestore/Protos/protos/firestore/local/target.proto
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,7 @@ message TargetGlobal {
// This is updated whenever our we get a TargetChange with a read_time and
// empty target_ids.
google.protobuf.Timestamp last_remote_snapshot_version = 3;

// On platforms that need it, holds the number of targets persisted.
int32 target_count = 4;
}
42 changes: 39 additions & 3 deletions Firestore/Source/Local/FSTLevelDBMigrations.mm
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,22 @@

#include "Firestore/Source/Local/FSTLevelDBMigrations.h"

#include <leveldb/db.h>
#include <leveldb/write_batch.h>
#include "leveldb/write_batch.h"

#import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
#import "Firestore/Source/Local/FSTLevelDB.h"
#import "Firestore/Source/Local/FSTLevelDBKey.h"
#import "Firestore/Source/Local/FSTLevelDBQueryCache.h"
#import "Firestore/Source/Local/FSTWriteGroup.h"
#import "Firestore/Source/Util/FSTAssert.h"

NS_ASSUME_NONNULL_BEGIN

// Current version of the schema defined in this file.
static FSTLevelDBSchemaVersion kSchemaVersion = 1;
static FSTLevelDBSchemaVersion kSchemaVersion = 2;

using leveldb::DB;
using leveldb::Iterator;
using leveldb::Status;
using leveldb::Slice;
using leveldb::WriteOptions;
Expand All @@ -57,6 +58,31 @@ static void SaveVersion(FSTLevelDBSchemaVersion version, FSTWriteGroup *group) {
[group setData:version_string forKey:key];
}

/**
* This function counts the number of targets that currently exist in the given db. It
* then reads the target global row, adds the count to the metadata from that row, and writes
* the metadata back.
*
* It assumes the metadata has already been written and is able to be read in this transaction.
*/
static void AddTargetCount(std::shared_ptr<DB> db, FSTWriteGroup *group) {
std::unique_ptr<Iterator> it(db->NewIterator([FSTLevelDB standardReadOptions]));
Slice start_key = [FSTLevelDBTargetKey keyPrefix];
it->Seek(start_key);

int32_t count = 0;
while (it->Valid() && it->key().starts_with(start_key)) {
count++;
it->Next();
}

FSTPBTargetGlobal *targetGlobal = [FSTLevelDBQueryCache readTargetMetadataFromDB:db];
FSTCAssert(targetGlobal != nil,
@"We should have a metadata row as it was added in an earlier migration");
targetGlobal.targetCount = count;
[group setMessage:targetGlobal forKey:[FSTLevelDBTargetGlobalKey key]];
}

@implementation FSTLevelDBMigrations

+ (FSTLevelDBSchemaVersion)schemaVersionForDB:(std::shared_ptr<DB>)db {
Expand All @@ -80,6 +106,16 @@ + (void)runMigrationsOnDB:(std::shared_ptr<DB>)db {
case 0:
EnsureTargetGlobal(db, group);
// Fallthrough
case 1:
// We need to make sure we have metadata, since we're going to read and modify it
// in this migration. Commit the current transaction and start a new one. Since we're
// committing, we need to save a version. It's safe to save this one, if we crash
// after saving we'll resume from this step when we try to migrate.
SaveVersion(1, group);
[group writeToDB:db];
group = [FSTWriteGroup groupWithAction:@"Migrations"];
AddTargetCount(db, group);
// Fallthrough
default:
if (currentVersion < kSchemaVersion) {
SaveVersion(kSchemaVersion, group);
Expand Down
54 changes: 39 additions & 15 deletions Firestore/Source/Local/FSTLevelDBQueryCache.mm
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

#include <leveldb/db.h>
#include <leveldb/write_batch.h>
#include <string>

#import "Firestore/Protos/objc/firestore/local/Target.pbobjc.h"
#import "Firestore/Source/Core/FSTQuery.h"
Expand Down Expand Up @@ -127,31 +126,50 @@ - (void)shutdown {
_db.reset();
}

- (void)addQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
- (void)saveQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
FSTTargetID targetID = queryData.targetID;
std::string key = [FSTLevelDBTargetKey keyWithTargetID:targetID];
[group setMessage:[self.serializer encodedQueryData:queryData] forKey:key];
}

- (void)saveMetadataInGroup:(FSTWriteGroup *)group {
[group setMessage:self.metadata forKey:[FSTLevelDBTargetGlobalKey key]];
}

- (BOOL)updateMetadataForQueryData:(FSTQueryData *)queryData {
BOOL updatedMetadata = NO;

if (queryData.targetID > self.metadata.highestTargetId) {
self.metadata.highestTargetId = queryData.targetID;
updatedMetadata = YES;
}

if (queryData.sequenceNumber > self.metadata.highestListenSequenceNumber) {
self.metadata.highestListenSequenceNumber = queryData.sequenceNumber;
updatedMetadata = YES;
}
return updatedMetadata;
}

- (void)addQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
[self saveQueryData:queryData group:group];

NSString *canonicalID = queryData.query.canonicalID;
std::string indexKey =
[FSTLevelDBQueryTargetKey keyWithCanonicalID:canonicalID targetID:targetID];
[FSTLevelDBQueryTargetKey keyWithCanonicalID:canonicalID targetID:queryData.targetID];
std::string emptyBuffer;
[group setData:emptyBuffer forKey:indexKey];

BOOL saveMetadata = NO;
FSTPBTargetGlobal *metadata = self.metadata;
if (targetID > metadata.highestTargetId) {
metadata.highestTargetId = targetID;
saveMetadata = YES;
}
self.metadata.targetCount += 1;
[self updateMetadataForQueryData:queryData];
[self saveMetadataInGroup:group];
}

if (queryData.sequenceNumber > metadata.highestListenSequenceNumber) {
metadata.highestListenSequenceNumber = queryData.sequenceNumber;
saveMetadata = YES;
}
- (void)updateQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
[self saveQueryData:queryData group:group];

if (saveMetadata) {
[group setMessage:metadata forKey:[FSTLevelDBTargetGlobalKey key]];
if ([self updateMetadataForQueryData:queryData]) {
[self saveMetadataInGroup:group];
}
}

Expand All @@ -166,6 +184,12 @@ - (void)removeQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
std::string indexKey =
[FSTLevelDBQueryTargetKey keyWithCanonicalID:queryData.query.canonicalID targetID:targetID];
[group removeMessageForKey:indexKey];
self.metadata.targetCount -= 1;
[self saveMetadataInGroup:group];
}

- (int32_t)count {
return self.metadata.targetCount;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Local/FSTLocalStore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
queryData = [queryData queryDataByReplacingSnapshotVersion:change.snapshotVersion
resumeToken:resumeToken];
self.targetIDs[targetIDNumber] = queryData;
[self.queryCache addQueryData:queryData group:group];
[self.queryCache updateQueryData:queryData group:group];
}
}];

Expand Down
14 changes: 14 additions & 0 deletions Firestore/Source/Local/FSTMemoryQueryCache.mm
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@ - (void)addQueryData:(FSTQueryData *)queryData group:(__unused FSTWriteGroup *)g
}
}

- (void)updateQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group {
self.queries[queryData.query] = queryData;
if (queryData.targetID > self.highestTargetID) {
self.highestTargetID = queryData.targetID;
}
if (queryData.sequenceNumber > self.highestListenSequenceNumber) {
self.highestListenSequenceNumber = queryData.sequenceNumber;
}
}

- (int32_t)count {
return (int32_t)[self.queries count];
}

- (void)removeQueryData:(FSTQueryData *)queryData group:(__unused FSTWriteGroup *)group {
[self.queries removeObjectForKey:queryData.query];
[self.references removeReferencesForID:queryData.targetID];
Expand Down
19 changes: 15 additions & 4 deletions Firestore/Source/Local/FSTQueryCache.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,29 @@ NS_ASSUME_NONNULL_BEGIN
group:(FSTWriteGroup *)group;

/**
* Adds or replaces an entry in the cache.
* Adds an entry in the cache.
*
* The cache key is extracted from `queryData.query`. If there is already a cache entry for the
* key, it will be replaced.
* The cache key is extracted from `queryData.query`. The key must not already exist in the cache.
*
* @param queryData An FSTQueryData instance to put in the cache.
* @param queryData A new FSTQueryData instance to put in the cache.
*/
- (void)addQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group;

/**
* Updates an entry in the cache.
*
* The cache key is extracted from `queryData.query`. The entry must already exist in the cache,
* and it will be replaced.
* @param queryData An FSTQueryData instance to replace an existing entry in the cache
*/
- (void)updateQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group;

/** Removes the cached entry for the given query data (no-op if no entry exists). */
- (void)removeQueryData:(FSTQueryData *)queryData group:(FSTWriteGroup *)group;

/** Returns the number of targets cached. */
- (int32_t)count;

/**
* Looks up an FSTQueryData entry in the cache.
*
Expand Down

0 comments on commit 5f5f808

Please sign in to comment.