Skip to content

Commit 2c53bb9

Browse files
authored
C++ migration: use C++ serializer for communicating with the local store (#4116)
After this PR is merged, the Objective-C serializers can be completely removed from the project. Also, this PR changes the `Message` class quite a bit; it's probably in its final form now. I did some performance testing locally and came to the conclusion that replacing Objective-C serializers with their C++ versions brings no performance downgrade; actually, there seems to be a very slight improvement. I tested writing large batches and reading large collections: * both are network-dominated; wall time is about twice longer than CPU time; * serialization represents a pretty small percentage of the total CPU time; * for writing, C++ serialization is a little more efficient, taking ~6% of total CPU time, compared to ~8% in the Objective-C version. I've seen similar results for reading. All integration tests pass, under Address Sanitizer as well.
1 parent 47afee7 commit 2c53bb9

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

44 files changed

+1011
-731
lines changed

Firestore/Example/Benchmarks/FSTLevelDBBenchmarkTests.mm

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,12 @@
1919

2020
#include <cstdint>
2121

22-
#import "Firestore/Source/Local/FSTLocalSerializer.h"
23-
#import "Firestore/Source/Remote/FSTSerializerBeta.h"
24-
2522
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
2623
#include "Firestore/core/src/firebase/firestore/local/leveldb_transaction.h"
24+
#include "Firestore/core/src/firebase/firestore/local/local_serializer.h"
2725
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
2826
#include "Firestore/core/src/firebase/firestore/model/types.h"
27+
#include "Firestore/core/src/firebase/firestore/remote/serializer.h"
2928
#include "Firestore/core/src/firebase/firestore/util/filesystem.h"
3029
#include "Firestore/core/src/firebase/firestore/util/path.h"
3130
#include "Firestore/core/src/firebase/firestore/util/string_format.h"
@@ -38,9 +37,11 @@
3837
using firebase::firestore::local::LevelDbRemoteDocumentKey;
3938
using firebase::firestore::local::LevelDbTargetDocumentKey;
4039
using firebase::firestore::local::LevelDbTransaction;
40+
using firebase::firestore::local::LocalSerializer;
4141
using firebase::firestore::model::DatabaseId;
4242
using firebase::firestore::model::DocumentKey;
4343
using firebase::firestore::model::TargetId;
44+
using firebase::firestore::remote::Serializer;
4445
using firebase::firestore::util::StringFormat;
4546
using firebase::firestore::util::Path;
4647

@@ -59,13 +60,13 @@
5960

6061
FSTLevelDB *LevelDBPersistence() {
6162
DatabaseId db_id("p", "d");
62-
auto remoteSerializer = [[FSTSerializerBeta alloc] initWithDatabaseID:db_id];
63-
auto serializer = [[FSTLocalSerializer alloc] initWithRemoteSerializer:remoteSerializer];
63+
Serializer remoteSerializer{db_id};
64+
LocalSerializer serializer{std::move(remoteSerializer)};
6465

6566
FSTLevelDB *db;
6667
Path path = util::TempDir().AppendUtf8("FSTLevelDBBenchmarkTests");
6768
util::Status status = [FSTLevelDB dbWithDirectory:std::move(path)
68-
serializer:serializer
69+
serializer:std::move(serializer)
6970
lruParams:local::LruParams::Disabled()
7071
ptr:&db];
7172
if (!status.ok()) {

Firestore/Example/FuzzTests/FuzzingTargets/FSTFuzzTestSerializer.mm

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#import "Firestore/Example/FuzzTests/FuzzingTargets/FSTFuzzTestSerializer.h"
2222

2323
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
24+
#include "Firestore/core/src/firebase/firestore/nanopb/message.h"
2425
#include "Firestore/core/src/firebase/firestore/nanopb/reader.h"
2526
#include "Firestore/core/src/firebase/firestore/remote/serializer.h"
2627

@@ -29,18 +30,17 @@
2930
namespace fuzzing {
3031

3132
using firebase::firestore::model::DatabaseId;
32-
using firebase::firestore::nanopb::Reader;
33+
using firebase::firestore::nanopb::StringReader;
3334
using firebase::firestore::remote::Serializer;
3435

3536
int FuzzTestDeserialization(const uint8_t *data, size_t size) {
3637
Serializer serializer{DatabaseId{"project"}};
3738

3839
@autoreleasepool {
3940
@try {
40-
Reader reader(data, size);
41-
google_firestore_v1_Value nanopb_proto{};
42-
reader.ReadNanopbMessage(google_firestore_v1_Value_fields, &nanopb_proto);
43-
serializer.DecodeFieldValue(&reader, nanopb_proto);
41+
StringReader reader{data, size};
42+
auto message = Message<google_firestore_v1_Value>::TryParse(&reader);
43+
serializer.DecodeFieldValue(&reader, *message);
4444
} @catch (...) {
4545
// Caught exceptions are ignored because the input might be malformed and
4646
// the deserialization might throw an error as intended. Fuzzing focuses on

Firestore/Example/Tests/Local/FSTLevelDBMigrationsTests.mm

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
NS_ASSUME_NONNULL_BEGIN
3838

3939
using firebase::firestore::Error;
40+
using firebase::firestore::firestore_client_TargetGlobal;
4041
using firebase::firestore::local::LevelDbCollectionParentKey;
4142
using firebase::firestore::local::LevelDbDir;
4243
using firebase::firestore::local::LevelDbDocumentMutationKey;
@@ -55,6 +56,7 @@
5556
using firebase::firestore::model::DocumentKey;
5657
using firebase::firestore::model::ListenSequenceNumber;
5758
using firebase::firestore::model::TargetId;
59+
using firebase::firestore::nanopb::Message;
5860
using firebase::firestore::testutil::Key;
5961
using firebase::firestore::util::OrderedCode;
6062
using firebase::firestore::util::Path;
@@ -88,12 +90,12 @@ - (void)tearDown {
8890
}
8991

9092
- (void)testAddsTargetGlobal {
91-
FSTPBTargetGlobal *metadata = LevelDbQueryCache::ReadMetadata(_db.get());
92-
XCTAssertNil(metadata, @"Not expecting metadata yet, we should have an empty db");
93+
auto metadata = LevelDbQueryCache::TryReadMetadata(_db.get());
94+
XCTAssert(!metadata, @"Not expecting metadata yet, we should have an empty db");
9395
LevelDbMigrations::RunMigrations(_db.get());
9496

95-
metadata = LevelDbQueryCache::ReadMetadata(_db.get());
96-
XCTAssertNotNil(metadata, @"Migrations should have added the metadata");
97+
metadata = LevelDbQueryCache::TryReadMetadata(_db.get());
98+
XCTAssert(metadata, @"Migrations should have added the metadata");
9799
}
98100

99101
- (void)testSetsVersionNumber {
@@ -168,9 +170,9 @@ - (void)testDropsTheQueryCache {
168170
ASSERT_FOUND(transaction, key);
169171
}
170172

171-
FSTPBTargetGlobal *metadata = LevelDbQueryCache::ReadMetadata(_db.get());
172-
XCTAssertNotNil(metadata, @"Metadata should have been added");
173-
XCTAssertEqual(metadata.targetCount, 0);
173+
auto metadata = LevelDbQueryCache::TryReadMetadata(_db.get());
174+
XCTAssert(metadata, @"Metadata should have been added");
175+
XCTAssertEqual(metadata.value()->target_count, 0);
174176
}
175177
}
176178

@@ -211,9 +213,9 @@ - (void)testAddsSentinelRows {
211213
LevelDbTransaction transaction(_db.get(), "Setup");
212214

213215
// Set up target global
214-
FSTPBTargetGlobal *metadata = LevelDbQueryCache::ReadMetadata(_db.get());
216+
auto metadata = LevelDbQueryCache::ReadMetadata(_db.get());
215217
// Expect that documents missing a row will get the new number
216-
metadata.highestListenSequenceNumber = new_sequence_number;
218+
metadata->highest_listen_sequence_number = new_sequence_number;
217219
transaction.Put(LevelDbTargetGlobalKey::Key(), metadata);
218220

219221
// Set up some documents (we only need the keys)

Firestore/core/src/firebase/firestore/core/firestore_client.mm

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,20 @@
2020
#include <memory>
2121
#include <utility>
2222

23-
#import "Firestore/Source/Local/FSTLocalSerializer.h"
24-
#import "Firestore/Source/Remote/FSTSerializerBeta.h"
25-
2623
#include "Firestore/core/src/firebase/firestore/api/settings.h"
2724
#include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h"
2825
#include "Firestore/core/src/firebase/firestore/core/database_info.h"
2926
#include "Firestore/core/src/firebase/firestore/core/event_manager.h"
3027
#include "Firestore/core/src/firebase/firestore/core/view.h"
3128
#include "Firestore/core/src/firebase/firestore/local/leveldb_persistence.h"
29+
#include "Firestore/core/src/firebase/firestore/local/local_serializer.h"
3230
#include "Firestore/core/src/firebase/firestore/local/memory_persistence.h"
3331
#include "Firestore/core/src/firebase/firestore/model/database_id.h"
3432
#include "Firestore/core/src/firebase/firestore/model/document_set.h"
3533
#include "Firestore/core/src/firebase/firestore/model/mutation.h"
3634
#include "Firestore/core/src/firebase/firestore/remote/datastore.h"
3735
#include "Firestore/core/src/firebase/firestore/remote/remote_store.h"
36+
#include "Firestore/core/src/firebase/firestore/remote/serializer.h"
3837
#include "Firestore/core/src/firebase/firestore/util/async_queue.h"
3938
#include "Firestore/core/src/firebase/firestore/util/delayed_constructor.h"
4039
#include "Firestore/core/src/firebase/firestore/util/exception.h"
@@ -59,6 +58,7 @@
5958
using auth::CredentialsProvider;
6059
using auth::User;
6160
using local::LevelDbPersistence;
61+
using local::LocalSerializer;
6262
using local::LocalStore;
6363
using local::LruParams;
6464
using local::MemoryPersistence;
@@ -71,6 +71,7 @@
7171
using model::OnlineState;
7272
using remote::Datastore;
7373
using remote::RemoteStore;
74+
using remote::Serializer;
7475
using util::AsyncQueue;
7576
using util::DelayedConstructor;
7677
using util::DelayedOperation;
@@ -153,13 +154,10 @@ new FirestoreClient(database_info, std::move(credentials_provider),
153154
Path dir = LevelDbPersistence::StorageDirectory(
154155
database_info_, LevelDbPersistence::AppDataDirectory());
155156

156-
FSTSerializerBeta* remote_serializer = [[FSTSerializerBeta alloc]
157-
initWithDatabaseID:database_info_.database_id()];
158-
FSTLocalSerializer* serializer =
159-
[[FSTLocalSerializer alloc] initWithRemoteSerializer:remote_serializer];
157+
Serializer remote_serializer{database_info_.database_id()};
160158

161159
auto created = LevelDbPersistence::Create(
162-
std::move(dir), serializer,
160+
std::move(dir), LocalSerializer{std::move(remote_serializer)},
163161
LruParams::WithCacheSize(settings.cache_size_bytes()));
164162
// If leveldb fails to start then just throw up our hands: the error is
165163
// unrecoverable. There's nothing an end-user can do and nearly all

Firestore/core/src/firebase/firestore/local/CMakeLists.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ cc_library(
8888
memory_remote_document_cache.mm
8989
mutation_queue.h
9090
persistence.h
91+
proto_sizer.cc
9192
proto_sizer.h
92-
proto_sizer.mm
9393
query_cache.h
9494
query_data.cc
9595
query_data.h

Firestore/core/src/firebase/firestore/local/leveldb_migrations.cc

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include "Firestore/core/src/firebase/firestore/local/memory_index_manager.h"
2626
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
2727
#include "Firestore/core/src/firebase/firestore/model/types.h"
28+
#include "Firestore/core/src/firebase/firestore/nanopb/message.h"
2829
#include "Firestore/core/src/firebase/firestore/nanopb/reader.h"
2930
#include "Firestore/core/src/firebase/firestore/nanopb/writer.h"
3031
#include "absl/strings/match.h"
@@ -39,7 +40,8 @@ using leveldb::Status;
3940
using leveldb::WriteOptions;
4041
using model::DocumentKey;
4142
using model::ResourcePath;
42-
using nanopb::Reader;
43+
using nanopb::Message;
44+
using nanopb::StringReader;
4345
using nanopb::Writer;
4446

4547
namespace {
@@ -113,8 +115,7 @@ void ClearQueryCache(leveldb::DB* db) {
113115
firestore_client_TargetGlobal target_global{};
114116

115117
nanopb::StringWriter writer;
116-
writer.WriteNanopbMessage(firestore_client_TargetGlobal_fields,
117-
&target_global);
118+
writer.Write(firestore_client_TargetGlobal_fields, &target_global);
118119
transaction.Put(LevelDbTargetGlobalKey::Key(), writer.Release());
119120

120121
SaveVersion(3, &transaction);
@@ -172,15 +173,14 @@ void RemoveAcknowledgedMutations(leveldb::DB* db) {
172173
for (; it->Valid() && absl::StartsWith(it->key(), mutation_queue_start);
173174
it->Next()) {
174175
HARD_ASSERT(key.Decode(it->key()), "Failed to decode mutation queue key");
175-
firestore_client_MutationQueue mutation_queue{};
176-
Reader reader(it->value());
177-
reader.ReadNanopbMessage(firestore_client_MutationQueue_fields,
178-
&mutation_queue);
176+
StringReader reader(it->value());
177+
auto mutation_queue =
178+
Message<firestore_client_MutationQueue>::TryParse(&reader);
179179
HARD_ASSERT(reader.status().ok(), "Failed to deserialize MutationQueue");
180180
RemoveMutationBatches(&transaction, key.user_id(),
181-
mutation_queue.last_acknowledged_batch_id);
181+
mutation_queue->last_acknowledged_batch_id);
182182
RemoveMutationDocuments(&transaction, key.user_id(),
183-
mutation_queue.last_acknowledged_batch_id);
183+
mutation_queue->last_acknowledged_batch_id);
184184
}
185185

186186
SaveVersion(5, &transaction);
@@ -195,11 +195,10 @@ model::ListenSequenceNumber GetHighestSequenceNumber(
195195
std::string bytes;
196196
transaction->Get(LevelDbTargetGlobalKey::Key(), &bytes);
197197

198-
firestore_client_TargetGlobal target_global{};
199-
Reader reader(bytes);
200-
reader.ReadNanopbMessage(firestore_client_TargetGlobal_fields,
201-
&target_global);
202-
return target_global.highest_listen_sequence_number;
198+
StringReader reader(bytes);
199+
auto target_global =
200+
Message<firestore_client_TargetGlobal>::TryParse(&reader);
201+
return target_global->highest_listen_sequence_number;
203202
}
204203

205204
/**

Firestore/core/src/firebase/firestore/local/leveldb_mutation_queue.h

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,36 +17,29 @@
1717
#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_LEVELDB_MUTATION_QUEUE_H_
1818
#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_LEVELDB_MUTATION_QUEUE_H_
1919

20-
#if !defined(__OBJC__)
21-
#error "For now, this file must only be included by ObjC source files."
22-
#endif // !defined(__OBJC__)
23-
24-
#import <Foundation/Foundation.h>
25-
2620
#include <set>
2721
#include <string>
2822
#include <vector>
2923

24+
#include "Firestore/Protos/nanopb/firestore/local/mutation.nanopb.h"
3025
#include "Firestore/core/include/firebase/firestore/timestamp.h"
3126
#include "Firestore/core/src/firebase/firestore/auth/user.h"
3227
#include "Firestore/core/src/firebase/firestore/local/leveldb_key.h"
3328
#include "Firestore/core/src/firebase/firestore/local/mutation_queue.h"
3429
#include "Firestore/core/src/firebase/firestore/model/document_key.h"
3530
#include "Firestore/core/src/firebase/firestore/model/document_key_set.h"
3631
#include "Firestore/core/src/firebase/firestore/model/types.h"
32+
#include "Firestore/core/src/firebase/firestore/nanopb/message.h"
3733
#include "absl/strings/string_view.h"
34+
#include "absl/types/optional.h"
3835
#include "leveldb/db.h"
3936

40-
@class FSTLocalSerializer;
41-
@class FSTPBMutationQueue;
42-
43-
NS_ASSUME_NONNULL_BEGIN
44-
4537
namespace firebase {
4638
namespace firestore {
4739
namespace local {
4840

4941
class LevelDbPersistence;
42+
class LocalSerializer;
5043

5144
/**
5245
* Returns one larger than the largest batch ID that has been stored. If there
@@ -58,7 +51,7 @@ class LevelDbMutationQueue : public MutationQueue {
5851
public:
5952
LevelDbMutationQueue(const auth::User& user,
6053
LevelDbPersistence* db,
61-
FSTLocalSerializer* serializer);
54+
LocalSerializer* serializer);
6255

6356
void Start() override;
6457

@@ -97,7 +90,7 @@ class LevelDbMutationQueue : public MutationQueue {
9790

9891
nanopb::ByteString GetLastStreamToken() override;
9992

100-
void SetLastStreamToken(const nanopb::ByteString& stream_token) override;
93+
void SetLastStreamToken(nanopb::ByteString stream_token) override;
10194

10295
private:
10396
/**
@@ -107,7 +100,7 @@ class LevelDbMutationQueue : public MutationQueue {
107100
std::vector<model::MutationBatch> AllMutationBatchesWithIds(
108101
const std::set<model::BatchId>& batch_ids);
109102

110-
std::string mutation_queue_key() {
103+
std::string mutation_queue_key() const {
111104
return LevelDbMutationQueueKey::Key(user_id_);
112105
}
113106

@@ -116,14 +109,16 @@ class LevelDbMutationQueue : public MutationQueue {
116109
}
117110

118111
/** Parses the MutationQueue metadata from the given LevelDB row contents. */
119-
FSTPBMutationQueue* _Nullable MetadataForKey(const std::string& key);
112+
nanopb::Message<firestore_client_MutationQueue> MetadataForKey(
113+
const std::string& key);
120114

121115
model::MutationBatch ParseMutationBatch(absl::string_view encoded);
122116

123117
// The LevelDbMutationQueue instance is owned by LevelDbPersistence.
124118
LevelDbPersistence* db_;
125119

126-
FSTLocalSerializer* serializer_;
120+
// Owned by LevelDbPersistence.
121+
LocalSerializer* serializer_ = nullptr;
127122

128123
/**
129124
* The normalized user_id (i.e. after converting null to empty) as used in our
@@ -143,13 +138,11 @@ class LevelDbMutationQueue : public MutationQueue {
143138
/**
144139
* A write-through cache copy of the metadata describing the current queue.
145140
*/
146-
FSTPBMutationQueue* _Nullable metadata_;
141+
nanopb::Message<firestore_client_MutationQueue> metadata_;
147142
};
148143

149144
} // namespace local
150145
} // namespace firestore
151146
} // namespace firebase
152147

153-
NS_ASSUME_NONNULL_END
154-
155148
#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_LOCAL_LEVELDB_MUTATION_QUEUE_H_

0 commit comments

Comments
 (0)