Skip to content

Keep track of number of queries in the query cache #776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 28 commits into from
Feb 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no separate storage for different logical tables in LevelDB so it's important to test that your key ranges are properly bounded. Please make sure to insert adversarial key prefixes to verify that your key prefix is properly constructed.

See here for an example:

- (void)testLoadNextBatchID_findsMaxAcrossUsers {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a key to test this, but it appears that the way we construct keys includes a table name and terminator in the prefix. I'm fairly certain that means as a general rule we're ok using a full tablename as a prefix for iteration bounds. There actually already exists an adversarial key, the target_global table/key (vs a table name of 'target'), but it is not explicit in the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right: I added the explicit terminators so that we couldn't be tricked by "food" when searching in the table "foo". However, having been bitten by this by failing to use the routines correctly I'm paranoid.

}

@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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty gross and is another poke at the idea that we should make WriteGroups into Transactions that support reading back pending values.

(This is not actionable feedback here; more of a reminder for myself.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Not sure it's really worth doing yet, but I can see how the current state is less than optimal.

[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