Skip to content

Add allMutationsAffectingDocumentKeys to FSTMutationQueue #1479

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.

Sign up for GitHub

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 3 commits into from
Jul 9, 2018
Merged

Add allMutationsAffectingDocumentKeys to FSTMutationQueue #1479

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c6c2a9c
Pod updates for Cocapods 1.5.3
wilhuff Jun 29, 2018
76bf96a
Add allMutationsAffectingDocumentKeys
wilhuff Jun 29, 2018
532a13a
Review feedback
wilhuff Jul 8, 2018
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
96 changes: 0 additions & 96 deletions Firestore/Example/Firestore.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,7 +1183,6 @@
54C9EDEE2040E16300A969CD /* Frameworks */,
54C9EDEF2040E16300A969CD /* Resources */,
EA424838F4A5DD7B337F57AB /* [CP] Embed Pods Frameworks */,
6BD54D799442CEB09349B73E /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand All @@ -1204,7 +1203,6 @@
6003F587195388D20070C39A /* Frameworks */,
6003F588195388D20070C39A /* Resources */,
1EE692C7509A98D7EB03CA51 /* [CP] Embed Pods Frameworks */,
A4BCE623F5E4C28728E5F17A /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand All @@ -1224,7 +1222,6 @@
6003F5AB195388D20070C39A /* Frameworks */,
6003F5AC195388D20070C39A /* Resources */,
329C25E418360CEF62F6CB2B /* [CP] Embed Pods Frameworks */,
263508FF7FD6CA4D6C3E685D /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand All @@ -1245,7 +1242,6 @@
6EDD3B4520BF247500C33877 /* Frameworks */,
6EDD3B4A20BF247500C33877 /* Resources */,
6EDD3B5720BF247500C33877 /* [CP] Embed Pods Frameworks */,
F5DFA8B0274B042DC1B00837 /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand All @@ -1266,7 +1262,6 @@
DE03B2D31F2149D600A30B9C /* Frameworks */,
DE03B2D81F2149D600A30B9C /* Resources */,
B7923D95031DB0DA112AAE9B /* [CP] Embed Pods Frameworks */,
5B2A669EEE88DF2205316429 /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand All @@ -1287,7 +1282,6 @@
DE0761E11F2FE611003233AF /* Frameworks */,
DE0761E21F2FE611003233AF /* Resources */,
04404E0DCBB886A40E3C7175 /* [CP] Embed Pods Frameworks */,
138396D16F5128E073E667C6 /* [CP] Copy Pods Resources */,
);
buildRules = (
);
Expand Down Expand Up @@ -1459,21 +1453,6 @@
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS-SwiftBuildTest/Pods-Firestore_Example_iOS-SwiftBuildTest-frameworks.sh\"\n";
showEnvVarsInLog = 0;
};
138396D16F5128E073E667C6 /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS-SwiftBuildTest/Pods-Firestore_Example_iOS-SwiftBuildTest-resources.sh\"\n";
showEnvVarsInLog = 0;
};
1EE692C7509A98D7EB03CA51 /* [CP] Embed Pods Frameworks */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
Expand Down Expand Up @@ -1510,21 +1489,6 @@
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS/Pods-Firestore_Example_iOS-frameworks.sh\"\n";
showEnvVarsInLog = 0;
};
263508FF7FD6CA4D6C3E685D /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Tests_iOS/Pods-Firestore_Tests_iOS-resources.sh\"\n";
showEnvVarsInLog = 0;
};
329C25E418360CEF62F6CB2B /* [CP] Embed Pods Frameworks */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
Expand Down Expand Up @@ -1567,36 +1531,6 @@
shellScript = "diff \"${PODS_PODFILE_DIR_PATH}/Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n # print error to STDERR\n echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n exit 1\nfi\n# This output is used by Xcode 'outputs' to avoid re-running this script phase.\necho \"SUCCESS\" > \"${SCRIPT_OUTPUT_FILE_0}\"\n";
showEnvVarsInLog = 0;
};
5B2A669EEE88DF2205316429 /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_IntegrationTests_iOS/Pods-Firestore_IntegrationTests_iOS-resources.sh\"\n";
showEnvVarsInLog = 0;
};
6BD54D799442CEB09349B73E /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS-resources.sh\"\n";
showEnvVarsInLog = 0;
};
6EDD3AD420BF247500C33877 /* [CP] Check Pods Manifest.lock */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
Expand Down Expand Up @@ -1669,21 +1603,6 @@
shellScript = "diff \"${PODS_PODFILE_DIR_PATH}/Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n # print error to STDERR\n echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n exit 1\nfi\n# This output is used by Xcode 'outputs' to avoid re-running this script phase.\necho \"SUCCESS\" > \"${SCRIPT_OUTPUT_FILE_0}\"\n";
showEnvVarsInLog = 0;
};
A4BCE623F5E4C28728E5F17A /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS/Pods-Firestore_Example_iOS-resources.sh\"\n";
showEnvVarsInLog = 0;
};
A827A009A65B69DC1B80EAD4 /* [CP] Check Pods Manifest.lock */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
Expand Down Expand Up @@ -1774,21 +1693,6 @@
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS/Pods-Firestore_Example_iOS-Firestore_SwiftTests_iOS-frameworks.sh\"\n";
showEnvVarsInLog = 0;
};
F5DFA8B0274B042DC1B00837 /* [CP] Copy Pods Resources */ = {
isa = PBXShellScriptBuildPhase;
buildActionMask = 2147483647;
files = (
);
inputPaths = (
);
name = "[CP] Copy Pods Resources";
outputPaths = (
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-Firestore_FuzzTests_iOS/Pods-Firestore_FuzzTests_iOS-resources.sh\"\n";
showEnvVarsInLog = 0;
};
/* End PBXShellScriptBuildPhase section */

/* Begin PBXSourcesBuildPhase section */
Expand Down
84 changes: 84 additions & 0 deletions Firestore/Example/Tests/Local/FSTMutationQueueTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,14 @@

#include "Firestore/core/src/firebase/firestore/auth/user.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
#include "Firestore/core/test/firebase/firestore/testutil/testutil.h"

namespace testutil = firebase::firestore::testutil;
using firebase::firestore::auth::User;
using firebase::firestore::model::DocumentKey;
using firebase::firestore::model::DocumentKeySet;
using firebase::firestore::testutil::Key;

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -315,6 +318,87 @@ - (void)testAllMutationBatchesAffectingDocumentKey {
});
}

- (void)testAllMutationBatchesAffectingDocumentKeys {
if ([self isTestBaseClass]) return;

self.persistence.run("testAllMutationBatchesAffectingDocumentKey", [&]() {
NSArray<FSTMutation *> *mutations = @[
FSTTestSetMutation(@"fob/bar",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultranit: could you replace the b in fob with a letter that has less resemblance to an o, say, x or k or y? It took me three passes to finally spot the difference between this element and the next one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. The idea is that this key precedes the others so I've chosen i.

@{ @"a" : @1 }),
FSTTestSetMutation(@"foo/bar",
@{ @"a" : @1 }),
FSTTestPatchMutation("foo/bar",
@{ @"b" : @1 }, {}),
FSTTestSetMutation(@"foo/bar/suffix/key",
@{ @"a" : @1 }),
FSTTestSetMutation(@"foo/baz",
@{ @"a" : @1 }),
FSTTestSetMutation(@"food/bar",
@{ @"a" : @1 })
];

// Store all the mutations.
NSMutableArray<FSTMutationBatch *> *batches = [NSMutableArray array];
for (FSTMutation *mutation in mutations) {
FSTMutationBatch *batch =
[self.mutationQueue addMutationBatchWithWriteTime:[FIRTimestamp timestamp]
mutations:@[ mutation ]];
[batches addObject:batch];
}

DocumentKeySet keys{
Key("foo/bar"),
Key("foo/baz"),
};

NSArray<FSTMutationBatch *> *expected = @[ batches[1], batches[2], batches[4] ];
NSArray<FSTMutationBatch *> *matches =
[self.mutationQueue allMutationBatchesAffectingDocumentKeys:keys];

XCTAssertEqualObjects(matches, expected);
});
}

- (void)testAllMutationBatchesAffectingDocumentKeys_handlesOverlap {
if ([self isTestBaseClass]) return;

self.persistence.run("testAllMutationBatchesAffectingDocumentKeys_handlesOverlap", [&]() {
NSMutableArray<FSTMutationBatch *> *batches = [NSMutableArray array];

NSArray<FSTMutation *> *group1 = @[
FSTTestSetMutation(@"foo/bar",
@{ @"a" : @1 }),
FSTTestSetMutation(@"foo/baz",
@{ @"a" : @1 }),
];
FSTMutationBatch *batch1 =
[self.mutationQueue addMutationBatchWithWriteTime:[FIRTimestamp timestamp]
mutations:group1];

NSArray<FSTMutation *> *group2 = @[ FSTTestSetMutation(@"food/bar", @{ @"a" : @1 }) ];
[self.mutationQueue addMutationBatchWithWriteTime:[FIRTimestamp timestamp] mutations:group2];

NSArray<FSTMutation *> *group3 = @[
FSTTestSetMutation(@"foo/bar",
@{ @"b" : @1 }),
];
FSTMutationBatch *batch3 =
[self.mutationQueue addMutationBatchWithWriteTime:[FIRTimestamp timestamp]
mutations:group3];

DocumentKeySet keys{
Key("foo/bar"),
Key("foo/baz"),
};

NSArray<FSTMutationBatch *> *expected = @[ batch1, batch3 ];
NSArray<FSTMutationBatch *> *matches =
[self.mutationQueue allMutationBatchesAffectingDocumentKeys:keys];

XCTAssertEqualObjects(matches, expected);
});
}

- (void)testAllMutationBatchesAffectingQuery {
if ([self isTestBaseClass]) return;

Expand Down
57 changes: 50 additions & 7 deletions Firestore/Source/Local/FSTLevelDBMutationQueue.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@

#include "Firestore/core/src/firebase/firestore/auth/user.h"
#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/model/resource_path.h"
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
Expand All @@ -46,6 +45,7 @@
using Firestore::StringView;
using firebase::firestore::auth::User;
using firebase::firestore::model::DocumentKey;
using firebase::firestore::model::DocumentKeySet;
using firebase::firestore::model::ResourcePath;
using leveldb::DB;
using leveldb::Iterator;
Expand Down Expand Up @@ -396,6 +396,39 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
return result;
}

- (NSArray<FSTMutationBatch *> *)allMutationBatchesAffectingDocumentKeys:
(const DocumentKeySet &)documentKeys {
NSString *userID = self.userID;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is storing this value in a variable intended as an optimization or readability improvement? In the latter case, I think moving it inline would be slightly better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a minor optimization. This is done fairly consistently throughout so I prefer to leave it as is.

As we rewrite to C++ this will become an instance variable reference.


// Take a pass through the document keys and collect the set of unique mutation batchIDs that
// affect them all. Some batches can affect more than one key.
std::set<FSTBatchID> batchIDs;

auto indexIterator = _db.currentTransaction->NewIterator();
FSTLevelDBDocumentMutationKey *rowKey = [[FSTLevelDBDocumentMutationKey alloc] init];
for (const DocumentKey &documentKey : documentKeys) {
std::string indexPrefix =
[FSTLevelDBDocumentMutationKey keyPrefixWithUserID:userID resourcePath:documentKey.path()];
indexIterator->Seek(indexPrefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very optional nit: I think you can move this expression to the initialization of for loop to avoid having a missing clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for (; indexIterator->Valid(); indexIterator->Next()) {
// Only consider rows matching exactly the specific key of interest. Note that because we
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: it's probably too much trouble, but I think a hypothetical example of how rows are laid out would make this comment easier to grasp and perhaps allow making it shorter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This helped tighten the text too.

// order by path first, and we order terminators before path separators, we'll encounter all
// the index rows for documentKey contiguously. In particular, all the rows for documentKey
// will occur before any rows for documents nested in a subcollection beneath documentKey so
// we can stop as soon as we hit any such row.
if (!absl::StartsWith(indexIterator->key(), indexPrefix) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be four variations of this check throughout the file. One could go away if the single-key version of this function were to be removed or rewritten to delegate to this function. Nevertheless, perhaps this check deserves its own named function? I can do this in a subsequent PR if you think it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trouble is that these aren't all the same. In three out of four cases we examine other parts of the row key after decoding it. In one case we consult the size of the key rather than an exact match. I'm also not keen on hiding the decoding the key too much.

The best way to solve this would probably be to add some mechanism for constructing the prefix including the marker that would start the next segment. That way we could do something like:

if (![rowKey decodeKey:indexIterator->key() withExactPrefix:suffixedIndexPrefix]) {
  break;
}

This would effectively combine the cheap indexPrefix check we have now with the exact DocumentKey check.

However, I'm struggling with what to call that thing, since we need both kinds: we need exact matches as we do in this case, but also any path matching the prefix as we need for allMutationBatchesAffectingQuery.

Given everything we have to do I'm inclined to just leave it as is. I'd rather not obsess about this too much--we need to make progress on the larger goals we have and this, while verbose, avoids being magical.

![rowKey decodeKey:indexIterator->key()] ||
DocumentKey{rowKey.documentKey} != documentKey) {
break;
}

batchIDs.insert(rowKey.batchID);
}
}

return [self allMutationBatchesWithBatchIDs:batchIDs];
}

- (NSArray<FSTMutationBatch *> *)allMutationBatchesAffectingQuery:(FSTQuery *)query {
HARD_ASSERT(![query isDocumentQuery], "Document queries shouldn't go down this path");
NSString *userID = self.userID;
Expand All @@ -417,11 +450,10 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
// index for more than a single document so the associated batchIDs will be neither necessarily
// unique nor in order. This means an efficient simultaneous scan isn't possible.
std::string indexPrefix =
[FSTLevelDBDocumentMutationKey keyPrefixWithUserID:self.userID resourcePath:queryPath];
[FSTLevelDBDocumentMutationKey keyPrefixWithUserID:userID resourcePath:queryPath];
auto indexIterator = _db.currentTransaction->NewIterator();
indexIterator->Seek(indexPrefix);

NSMutableArray *result = [NSMutableArray array];
FSTLevelDBDocumentMutationKey *rowKey = [[FSTLevelDBDocumentMutationKey alloc] init];

// Collect up unique batchIDs encountered during a scan of the index. Use a set<FSTBatchID> to
Expand All @@ -430,7 +462,7 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
// This method is faster than performing lookups of the keys with _db->Get and keeping a hash of
// batchIDs that have already been looked up. The performance difference is minor for small
// numbers of keys but > 30% faster for larger numbers of keys.
std::set<FSTBatchID> uniqueBatchIds;
std::set<FSTBatchID> uniqueBatchIDs;
for (; indexIterator->Valid(); indexIterator->Next()) {
if (!absl::StartsWith(indexIterator->key(), indexPrefix) ||
![rowKey decodeKey:indexIterator->key()]) {
Expand All @@ -444,14 +476,25 @@ - (nullable FSTMutationBatch *)nextMutationBatchAfterBatchID:(FSTBatchID)batchID
continue;
}

uniqueBatchIds.insert(rowKey.batchID);
uniqueBatchIDs.insert(rowKey.batchID);
}

return [self allMutationBatchesWithBatchIDs:uniqueBatchIDs];
}

/**
* Constructs an array of matching batches, sorted by batchID to ensure that multiple mutations
* affecting the same document key are applied in order.
*/
- (NSArray<FSTMutationBatch *> *)allMutationBatchesWithBatchIDs:
(const std::set<FSTBatchID> &)batchIDs {
NSMutableArray *result = [NSMutableArray array];
NSString *userID = self.userID;

// Given an ordered set of unique batchIDs perform a skipping scan over the main table to find
// the mutation batches.
auto mutationIterator = _db.currentTransaction->NewIterator();

for (FSTBatchID batchID : uniqueBatchIds) {
for (FSTBatchID batchID : batchIDs) {
std::string mutationKey = [FSTLevelDBMutationKey keyWithUserID:userID batchID:batchID];
mutationIterator->Seek(mutationKey);
if (!mutationIterator->Valid() || mutationIterator->key() != mutationKey) {
Expand Down
Loading