Skip to content

Eliminate unnecessary DocumentKey->FSTDocumentKey conversions #1507

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 5 commits into from
Jul 10, 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
13 changes: 7 additions & 6 deletions Firestore/Example/Tests/Integration/API/FIRWriteBatchTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ int64_t GetCurrentMemoryUsedInMb() {
const auto errorCode =
task_info(mach_task_self(), MACH_TASK_BASIC_INFO, (task_info_t)&taskInfo, &taskInfoSize);
if (errorCode == KERN_SUCCESS) {
const int bytesInMegabyte = 1000 * 1000;
const int bytesInMegabyte = 1024 * 1024;
return taskInfo.resident_size / bytesInMegabyte;
}
return -1;
Expand All @@ -349,8 +349,9 @@ - (void)testReasonableMemoryUsageForLotsOfMutations {
FIRDocumentReference *mainDoc = [self documentRef];
FIRWriteBatch *batch = [mainDoc.firestore batch];

// >= 500 mutations will be rejected, so use 500-1 mutations
for (int i = 0; i != 500 - 1; ++i) {
// > 500 mutations will be rejected.
const int maxMutations = 500;
for (int i = 0; i != maxMutations; ++i) {
FIRDocumentReference *nestedDoc = [[mainDoc collectionWithPath:@"nested"] documentWithAutoID];
// The exact data doesn't matter; what is important is the large number of mutations.
[batch setData:@{
Expand All @@ -367,9 +368,9 @@ - (void)testReasonableMemoryUsageForLotsOfMutations {
const int64_t memoryUsedAfterCommitMb = GetCurrentMemoryUsedInMb();
XCTAssertNotEqual(memoryUsedAfterCommitMb, -1);
const int64_t memoryDeltaMb = memoryUsedAfterCommitMb - memoryUsedBeforeCommitMb;
// This by its nature cannot be a precise value. In debug mode, the increase on simulator
// seems to be around 90 MB. A regression would be on the scale of 500Mb.
XCTAssertLessThan(memoryDeltaMb, 150);
// This by its nature cannot be a precise value. Runs on simulator seem to give an increase of
// 10MB in debug mode pretty consistently. A regression would be on the scale of 500Mb.
XCTAssertLessThan(memoryDeltaMb, 20);

[expectation fulfill];
}];
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Core/FSTView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ - (BOOL)isEqual:(id)other {
return NO;
}
FSTLimboDocumentChange *otherChange = (FSTLimboDocumentChange *)other;
return self.type == otherChange.type && [self.key isEqual:otherChange.key];
return self.type == otherChange.type && self.key == otherChange.key;
}

- (NSUInteger)hash {
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Local/FSTDocumentReference.mm
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ - (BOOL)isEqual:(id)other {

FSTDocumentReference *reference = (FSTDocumentReference *)other;

return [self.key isEqualToKey:reference.key] && self.ID == reference.ID;
return self.key == reference.key && self.ID == reference.ID;
}

- (NSUInteger)hash {
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Local/FSTLevelDBRemoteDocumentCache.mm
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ - (FSTMaybeDocument *)decodeMaybeDocument:(absl::string_view)encoded
}

FSTMaybeDocument *maybeDocument = [self.serializer decodedMaybeDocument:proto];
HARD_ASSERT([maybeDocument.key isEqualToKey:documentKey],
HARD_ASSERT(maybeDocument.key == documentKey,
"Read document has key (%s) instead of expected key (%s).",
maybeDocument.key.ToString(), documentKey.ToString());
return maybeDocument;
Expand Down
4 changes: 2 additions & 2 deletions Firestore/Source/Model/FSTDocument.mm
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ - (BOOL)isEqual:(id)other {
}

FSTDocument *otherDoc = other;
return [self.key isEqual:otherDoc.key] && self.version == otherDoc.version &&
return self.key == otherDoc.key && self.version == otherDoc.version &&
[self.data isEqual:otherDoc.data] && self.hasLocalMutations == otherDoc.hasLocalMutations;
}

Expand Down Expand Up @@ -142,7 +142,7 @@ - (BOOL)isEqual:(id)other {
}

FSTDocument *otherDoc = other;
return [self.key isEqual:otherDoc.key] && self.version == otherDoc.version;
return self.key == otherDoc.key && self.version == otherDoc.version;
}

- (NSUInteger)hash {
Expand Down
11 changes: 11 additions & 0 deletions Firestore/Source/Model/FSTDocumentKey.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@

#include "Firestore/core/src/firebase/firestore/model/resource_path.h"

// Using forward declaration to avoid circular dependency (`document_key.h` includes this header).`
namespace firebase {
namespace firestore {
namespace model {
class DocumentKey;
}
}
}

NS_ASSUME_NONNULL_BEGIN

/** FSTDocumentKey represents the location of a document in the Firestore database. */
Expand All @@ -33,6 +42,8 @@ NS_ASSUME_NONNULL_BEGIN
* @return A new instance of FSTDocumentKey.
*/
+ (instancetype)keyWithPath:(firebase::firestore::model::ResourcePath)path;

+ (instancetype)keyWithDocumentKey:(const firebase::firestore::model::DocumentKey &)documentKey;
/**
* Creates and returns a new document key with a path with the given segments.
*
Expand Down
29 changes: 16 additions & 13 deletions Firestore/Source/Model/FSTDocumentKey.mm
Original file line number Diff line number Diff line change
Expand Up @@ -21,26 +21,32 @@

#import "Firestore/Source/Core/FSTFirestoreClient.h"

#include "Firestore/core/src/firebase/firestore/model/resource_path.h"
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
#include "Firestore/core/src/firebase/firestore/util/hashing.h"
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"

namespace util = firebase::firestore::util;
using firebase::firestore::model::ResourcePath;
using firebase::firestore::model::DocumentKey;

NS_ASSUME_NONNULL_BEGIN

@interface FSTDocumentKey () {
/** The path to the document. */
ResourcePath _path;
// Forward most of the logic to the C++ implementation until FSTDocumentKey usages are completely
// migrated.
DocumentKey _delegate;
}
@end

@implementation FSTDocumentKey

+ (instancetype)keyWithPath:(ResourcePath)path {
return [[FSTDocumentKey alloc] initWithPath:std::move(path)];
return [[FSTDocumentKey alloc] initWithDocumentKey:DocumentKey{path}];
}

+ (instancetype)keyWithDocumentKey:(const firebase::firestore::model::DocumentKey &)documentKey {
return [[FSTDocumentKey alloc] initWithDocumentKey:documentKey];
}

+ (instancetype)keyWithSegments:(std::initializer_list<std::string>)segments {
Expand All @@ -52,12 +58,9 @@ + (instancetype)keyWithPathString:(NSString *)resourcePath {
}

/** Designated initializer. */
- (instancetype)initWithPath:(ResourcePath)path {
HARD_ASSERT([FSTDocumentKey isDocumentKey:path], "invalid document key path: %s",
path.CanonicalString());

- (instancetype)initWithDocumentKey:(const DocumentKey &)key {
if (self = [super init]) {
_path = path;
_delegate = key;
}
return self;
}
Expand All @@ -73,11 +76,11 @@ - (BOOL)isEqual:(id)object {
}

- (NSUInteger)hash {
return util::Hash(_path);
return _delegate.Hash();
}

- (NSString *)description {
return [NSString stringWithFormat:@"<FSTDocumentKey: %s>", _path.CanonicalString().c_str()];
return [NSString stringWithFormat:@"<FSTDocumentKey: %s>", _delegate.ToString().c_str()];
}

/** Implements NSCopying without actually copying because FSTDocumentKeys are immutable. */
Expand All @@ -100,11 +103,11 @@ + (NSComparator)comparator {
}

+ (BOOL)isDocumentKey:(const ResourcePath &)path {
return path.size() % 2 == 0;
return DocumentKey::IsDocumentKey(path);
}

- (const ResourcePath &)path {
return _path;
return _delegate.path();
}

@end
Expand Down
17 changes: 8 additions & 9 deletions Firestore/Source/Model/FSTMutation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ - (BOOL)isEqual:(id)other {
}

FSTSetMutation *otherMutation = (FSTSetMutation *)other;
return [self.key isEqual:otherMutation.key] && [self.value isEqual:otherMutation.value] &&
return self.key == otherMutation.key && [self.value isEqual:otherMutation.value] &&
self.precondition == otherMutation.precondition;
}

Expand Down Expand Up @@ -173,7 +173,7 @@ - (nullable FSTMaybeDocument *)applyTo:(nullable FSTMaybeDocument *)maybeDoc
[maybeDoc class]);
FSTDocument *doc = (FSTDocument *)maybeDoc;

HARD_ASSERT([doc.key isEqual:self.key], "Can only set a document with the same key");
HARD_ASSERT(doc.key == self.key, "Can only set a document with the same key");
return [FSTDocument documentWithData:self.value
key:doc.key
version:doc.version
Expand Down Expand Up @@ -212,7 +212,7 @@ - (BOOL)isEqual:(id)other {
}

FSTPatchMutation *otherMutation = (FSTPatchMutation *)other;
return [self.key isEqual:otherMutation.key] && self.fieldMask == otherMutation.fieldMask &&
return self.key == otherMutation.key && self.fieldMask == otherMutation.fieldMask &&
[self.value isEqual:otherMutation.value] &&
self.precondition == otherMutation.precondition;
}
Expand Down Expand Up @@ -259,7 +259,7 @@ - (nullable FSTMaybeDocument *)applyTo:(nullable FSTMaybeDocument *)maybeDoc
[maybeDoc class]);
FSTDocument *doc = (FSTDocument *)maybeDoc;

HARD_ASSERT([doc.key isEqual:self.key], "Can only patch a document with the same key");
HARD_ASSERT(doc.key == self.key, "Can only patch a document with the same key");

FSTObjectValue *newData = [self patchObjectValue:doc.data];
return [FSTDocument documentWithData:newData
Expand Down Expand Up @@ -312,8 +312,7 @@ - (BOOL)isEqual:(id)other {
}

FSTTransformMutation *otherMutation = (FSTTransformMutation *)other;
return [self.key isEqual:otherMutation.key] &&
self.fieldTransforms == otherMutation.fieldTransforms &&
return self.key == otherMutation.key && self.fieldTransforms == otherMutation.fieldTransforms &&
self.precondition == otherMutation.precondition;
}

Expand Down Expand Up @@ -355,7 +354,7 @@ - (nullable FSTMaybeDocument *)applyTo:(nullable FSTMaybeDocument *)maybeDoc
[maybeDoc class]);
FSTDocument *doc = (FSTDocument *)maybeDoc;

HARD_ASSERT([doc.key isEqual:self.key], "Can only transform a document with the same key");
HARD_ASSERT(doc.key == self.key, "Can only transform a document with the same key");

BOOL hasLocalMutations = (mutationResult == nil);
NSArray<FSTFieldValue *> *transformResults;
Expand Down Expand Up @@ -460,7 +459,7 @@ - (BOOL)isEqual:(id)other {
}

FSTDeleteMutation *otherMutation = (FSTDeleteMutation *)other;
return [self.key isEqual:otherMutation.key] && self.precondition == otherMutation.precondition;
return self.key == otherMutation.key && self.precondition == otherMutation.precondition;
}

- (NSUInteger)hash {
Expand Down Expand Up @@ -488,7 +487,7 @@ - (nullable FSTMaybeDocument *)applyTo:(nullable FSTMaybeDocument *)maybeDoc
}

if (maybeDoc) {
HARD_ASSERT([maybeDoc.key isEqual:self.key], "Can only delete a document with the same key");
HARD_ASSERT(maybeDoc.key == self.key, "Can only delete a document with the same key");
}

return [FSTDeletedDocument documentWithKey:self.key version:SnapshotVersion::None()];
Expand Down
4 changes: 2 additions & 2 deletions Firestore/Source/Model/FSTMutationBatch.mm
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ - (NSString *)description {
- (FSTMaybeDocument *_Nullable)applyTo:(FSTMaybeDocument *_Nullable)maybeDoc
documentKey:(const DocumentKey &)documentKey
mutationBatchResult:(FSTMutationBatchResult *_Nullable)mutationBatchResult {
HARD_ASSERT(!maybeDoc || [maybeDoc.key isEqualToKey:documentKey],
HARD_ASSERT(!maybeDoc || maybeDoc.key == documentKey,
"applyTo: key %s doesn't match maybeDoc key %s", documentKey.ToString(),
maybeDoc.key.ToString());
FSTMaybeDocument *baseDoc = maybeDoc;
Expand All @@ -90,7 +90,7 @@ - (FSTMaybeDocument *_Nullable)applyTo:(FSTMaybeDocument *_Nullable)maybeDoc
for (NSUInteger i = 0; i < self.mutations.count; i++) {
FSTMutation *mutation = self.mutations[i];
FSTMutationResult *_Nullable mutationResult = mutationBatchResult.mutationResults[i];
if ([mutation.key isEqualToKey:documentKey]) {
if (mutation.key == documentKey) {
maybeDoc = [mutation applyTo:maybeDoc
baseDocument:baseDoc
localWriteTime:self.localWriteTime
Expand Down
2 changes: 1 addition & 1 deletion Firestore/core/src/firebase/firestore/model/document_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class DocumentKey {
}

operator FSTDocumentKey*() const {
return [FSTDocumentKey keyWithPath:path()];
return [FSTDocumentKey keyWithDocumentKey:*this];
}

NSUInteger Hash() const {
Expand Down