pw_protobuf: better use of blob count in encoder
Repeated nested messages would each get their own blob, which did not
scale very well for some messages.
This is a small fix to help some users while waiting for an improved
pw_protobuf encoder.
Note: the second template argument for the NestedEncoder is now moot,
and should always be set to the same as the first. In order not
to break any code relying on this, it has been left alone.
Testing done:
- tested on a proto that would previously cause a RESOURCE_EXHAUSTED
error, now properly encodes
Change-Id: I8bbaf4c0769dcb0e6f6e9f727546bb9145240172
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/26500
Commit-Queue: Paul Mathieu <paulmathieu@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
diff --git a/pw_protobuf/encoder.cc b/pw_protobuf/encoder.cc
index 66174fc..6ff8c7c 100644
--- a/pw_protobuf/encoder.cc
+++ b/pw_protobuf/encoder.cc
@@ -123,15 +123,24 @@
SizeType child_size = *blob_stack_[--depth_];
IncreaseParentSize(child_size + VarintSizeBytes(child_size));
+ // Encode the child
+ if (Status status = EncodeFrom(blob_count_ - 1).status(); !status.ok()) {
+ encode_status_ = status;
+ return encode_status_;
+ }
+ blob_count_--;
+
return Status::Ok();
}
-Result<ConstByteSpan> Encoder::Encode() {
+Result<ConstByteSpan> Encoder::Encode() { return EncodeFrom(0); }
+
+Result<ConstByteSpan> Encoder::EncodeFrom(size_t blob) {
if (!encode_status_.ok()) {
return encode_status_;
}
- if (blob_count_ == 0) {
+ if (blob >= blob_count_) {
// If there are no nested blobs, the buffer already contains a valid proto.
return Result<ConstByteSpan>(buffer_.first(EncodedSize()));
}
@@ -143,7 +152,6 @@
// Starting from the first blob, encode each size field as a varint and
// shift all subsequent data downwards.
- unsigned int blob = 0;
size_cursor = blob_locations_[blob];
std::byte* write_cursor = read_cursor;
diff --git a/pw_protobuf/encoder_test.cc b/pw_protobuf/encoder_test.cc
index 605467b..a7d04b6 100644
--- a/pw_protobuf/encoder_test.cc
+++ b/pw_protobuf/encoder_test.cc
@@ -138,7 +138,7 @@
TEST(Encoder, Nested) {
std::byte encode_buffer[128];
- NestedEncoder<5, 10> encoder(encode_buffer);
+ NestedEncoder<5, 5> encoder(encode_buffer);
// TestProto test_proto;
// test_proto.magic_number = 42;
@@ -222,7 +222,7 @@
TEST(Encoder, NestedDepthLimit) {
std::byte encode_buffer[128];
- NestedEncoder<2, 10> encoder(encode_buffer);
+ NestedEncoder<2, 2> encoder(encode_buffer);
// One level of nesting.
EXPECT_EQ(encoder.Push(2), Status::Ok());
@@ -239,7 +239,7 @@
TEST(Encoder, NestedBlobLimit) {
std::byte encode_buffer[128];
- NestedEncoder<5, 3> encoder(encode_buffer);
+ NestedEncoder<3, 3> encoder(encode_buffer);
// Write first blob.
EXPECT_EQ(encoder.Push(1), Status::Ok());
@@ -255,10 +255,9 @@
// End second blob.
EXPECT_EQ(encoder.Pop(), Status::Ok());
- // Write fourth blob: error!.
- EXPECT_EQ(encoder.Push(4), Status::ResourceExhausted());
- // Nothing to pop.
- EXPECT_EQ(encoder.Pop(), Status::ResourceExhausted());
+ // Write fourth blob: OK
+ EXPECT_EQ(encoder.Push(4), Status::Ok());
+ EXPECT_EQ(encoder.Pop(), Status::Ok());
}
TEST(Encoder, RepeatedField) {
diff --git a/pw_protobuf/public/pw_protobuf/encoder.h b/pw_protobuf/public/pw_protobuf/encoder.h
index c859009..4277672 100644
--- a/pw_protobuf/public/pw_protobuf/encoder.h
+++ b/pw_protobuf/public/pw_protobuf/encoder.h
@@ -352,6 +352,9 @@
return size_bytes;
}
+ // Do the actual (potentially partial) encoding. Also used in Pop().
+ Result<ConstByteSpan> EncodeFrom(size_t blob);
+
// The buffer into which the proto is encoded.
ByteSpan buffer_;
std::byte* cursor_;
@@ -369,7 +372,7 @@
};
// A proto encoder with its own blob stack.
-template <size_t kMaxNestedDepth = 1, size_t kMaxBlobs = 1>
+template <size_t kMaxNestedDepth = 1, size_t kMaxBlobs = kMaxNestedDepth>
class NestedEncoder : public Encoder {
public:
NestedEncoder(ByteSpan buffer) : Encoder(buffer, blobs_, stack_) {}
