Skip to content

Commit e241a9a

Browse files
authored
Optimize DocumentKey comparisons (#4123)
This came up while I was investigating the effect of new C++ serializers on performance. When writing large batches, one of the hottest functions turned out to be `firebase::firestore::util::operator==(const DocumentKey& lhs, const DocumentKey& rhs)`, accounting for 15-20% of total CPU time. There are two reasons why comparing keys is currently not very efficient: * `BasePath::CompareTo`, to which this comparison eventually resolves, mistakenly uses the general-purpose `util::Compare` function rather than the more efficient `util::CompareContainer`; * the default implementation of `Comparable::operator==` results in two comparisons per element ("less-than" followed by "greater-than"), even though one would be sufficient. Fixing these issues results in 8-10% CPU time improvement when writing large batches. To work around the latter, this PR defines a new mixin `InequalityComparable` that is exactly the same as `Comparable`, except it doesn't define `operator==`, forcing the inheriting class to implement it manually. Some measurements (all done on the emulator in release mode with no logging) follow; the test case was to write 10 batches of 500 very small documents. Note that I found wall clock to fluctuate too much to be reliable and instead use the measurements from XCode's Time Profiler, which (presumably) shows CPU time. Each measurement shows total CPU time spent on `firebase::firestore::util::operator==` (all instantiations, not just for `DocumentKey`), followed by the percentage of time spent in this function compared to total runtime: * no optimization: * 275ms/18.8%; * 245ms/16.9%; * 252ms/18.5%. * `CompareContainer` optimization: * 124ms/9.7%; * 110ms/8.5%; * 128ms/10.0%. * `CompareContainer` + `ComparableWithEqual` optimizations: * 95ms/7.3%; * 100ms/8.0%; * 112ms/9.2%.
1 parent 2a45cf6 commit e241a9a

File tree

4 files changed

+38
-3
lines changed

4 files changed

+38
-3
lines changed

Firestore/core/src/firebase/firestore/model/base_path.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,11 @@ class BasePath {
151151
}
152152

153153
util::ComparisonResult CompareTo(const T& rhs) const {
154-
return util::Compare(segments_, rhs.segments_);
154+
return util::CompareContainer(segments_, rhs.segments_);
155+
}
156+
157+
friend bool operator==(const BasePath& lhs, const BasePath& rhs) {
158+
return lhs.segments_ == rhs.segments_;
155159
}
156160

157161
size_t Hash() const {

Firestore/core/src/firebase/firestore/model/document_key.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace model {
3535
/**
3636
* DocumentKey represents the location of a document in the Firestore database.
3737
*/
38-
class DocumentKey : public util::Comparable<DocumentKey> {
38+
class DocumentKey : public util::InequalityComparable<DocumentKey> {
3939
public:
4040
/** Creates a "blank" document key not associated with any document. */
4141
DocumentKey() : path_{std::make_shared<ResourcePath>()} {
@@ -70,6 +70,10 @@ class DocumentKey : public util::Comparable<DocumentKey> {
7070

7171
util::ComparisonResult CompareTo(const DocumentKey& other) const;
7272

73+
friend bool operator==(const DocumentKey& lhs, const DocumentKey& rhs) {
74+
return lhs.path() == rhs.path();
75+
}
76+
7377
size_t Hash() const {
7478
return util::Hash(ToString());
7579
}

Firestore/core/src/firebase/firestore/model/resource_path.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ namespace model {
3434
* within Firestore. Immutable; all instances are fully independent.
3535
*/
3636
class ResourcePath : public impl::BasePath<ResourcePath>,
37-
public util::Comparable<ResourcePath> {
37+
public util::InequalityComparable<ResourcePath> {
3838
public:
3939
ResourcePath() = default;
4040
/** Constructs the path from segments. */

Firestore/core/src/firebase/firestore/util/comparison.h

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,33 @@ class Comparable {
353353
}
354354
};
355355

356+
/**
357+
* Same as `Comparable`, but deliberately not defining `operator==`, which
358+
* instead is left for the class inheriting from this mixin to implement. This
359+
* is an optimization that avoids doing an extra comparison when comparing for
360+
* equality (`Comparable` would have to check for both "less-than" and
361+
* "greater-than" to determine whether two values are equal).
362+
*/
363+
template <typename T>
364+
class InequalityComparable {
365+
public:
366+
friend bool operator!=(const T& lhs, const T& rhs) {
367+
return !(lhs == rhs);
368+
}
369+
friend bool operator<(const T& lhs, const T& rhs) {
370+
return Ascending(lhs.CompareTo(rhs));
371+
}
372+
friend bool operator>(const T& lhs, const T& rhs) {
373+
return Descending(lhs.CompareTo(rhs));
374+
}
375+
friend bool operator<=(const T& lhs, const T& rhs) {
376+
return !(rhs < lhs);
377+
}
378+
friend bool operator>=(const T& lhs, const T& rhs) {
379+
return !(lhs < rhs);
380+
}
381+
};
382+
356383
} // namespace util
357384
} // namespace firestore
358385
} // namespace firebase

0 commit comments

Comments
 (0)