Move around zero-initialization for static flatbuffers
Trying to manually zero-initialize all the various padding was not
getting enough stuff initialized, and probably wasn't a meaningful
performance improvement anyways.
This was caught by the memory sanitizers on non-flatbuffers specific
tests; as such, add checks to the static_flatbuffers_test that should
catch these issues in the future.
Change-Id: I448565df097edbdc65085e991ee0ba9b0e23e2a2
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>
diff --git a/aos/flatbuffers/builder.h b/aos/flatbuffers/builder.h
index b556ed3..6999649 100644
--- a/aos/flatbuffers/builder.h
+++ b/aos/flatbuffers/builder.h
@@ -19,9 +19,28 @@
class Builder final : public ResizeableObject {
public:
static constexpr size_t kBufferSize = T::kUnalignedBufferSize;
+ // Note on memory initialization: We zero-initialize all the memory that we
+ // create at the start. While this can be overkill, it is simpler to manage
+ // the alternatives, and we don't currently have a clear performance need for
+ // doing this more piecemeal. Note that the memory that this zero-initializes
+ // falls into three categories:
+ // 1. Padding that we need to zero-initialize (arguably we could get away
+ // without initializing our padding, but leaving uninitialized memory
+ // floating around generally isn't great, especially since we generally
+ // want to be able to compress flatbuffer messages efficiently).
+ // 2. Memory that will end up getting used.
+ // 3. Memory corresponding to sub-tables/vectors that may or may not end up
+ // getting used; if it is not used, we want it to get zero-initialized
+ // since it will effectively be padding. If it is used, then the
+ // zero-initialization is redundant.
+ // For messages with large byte buffers (e.g., for camera messages), we
+ // typically expect that the user will end up dynamically resizing the buffer
+ // rather than having the length statically set. In those cases, the user
+ // still has the ability to select whether or not the new memory gets
+ // zero-initialized.
Builder(Allocator *allocator)
: ResizeableObject(
- allocator->AllocateOrDie(kBufferSize, T::kAlign, SetZero::kNo),
+ allocator->AllocateOrDie(kBufferSize, T::kAlign, SetZero::kYes),
allocator),
flatbuffer_start_(BufferStart(buffer_)),
flatbuffer_(internal::GetSubSpan(buffer_, flatbuffer_start_, T::kSize),
@@ -30,7 +49,7 @@
}
Builder(std::unique_ptr<Allocator> allocator)
: ResizeableObject(
- allocator->AllocateOrDie(kBufferSize, T::kAlign, SetZero::kNo),
+ allocator->AllocateOrDie(kBufferSize, T::kAlign, SetZero::kYes),
std::move(allocator)),
flatbuffer_start_(BufferStart(buffer_)),
flatbuffer_(internal::GetSubSpan(buffer_, flatbuffer_start_, T::kSize),
diff --git a/aos/flatbuffers/static_flatbuffers_test.cc b/aos/flatbuffers/static_flatbuffers_test.cc
index ed737cb..61ec118 100644
--- a/aos/flatbuffers/static_flatbuffers_test.cc
+++ b/aos/flatbuffers/static_flatbuffers_test.cc
@@ -55,6 +55,14 @@
}
return nullptr;
}
+
+// Accesses all the values in the supplied span. Used to ensure that memory
+// sanitizers can observe uninitialized memory.
+void TestMemory(std::span<uint8_t> memory) {
+ std::stringstream str;
+ internal::DebugBytes(memory, str);
+ EXPECT_LT(0u, str.view().size());
+}
} // namespace
class StaticFlatbuffersTest : public ::testing::Test {
@@ -204,6 +212,7 @@
EXPECT_EQ(971, object->AsFlatbuffer().baz());
EXPECT_EQ(R"json({ "foo": 123, "baz": 971.0 })json",
aos::FlatbufferToJson(builder.AsFlatbufferSpan()));
+ TestMemory(builder.buffer());
}
{
// aos::FixedAllocator allocator(TestTableStatic::kUnalignedBufferSize);
@@ -629,6 +638,7 @@
ASSERT_FALSE(unspecified_vector->emplace_back(3));
ASSERT_TRUE(builder.AsFlatbufferSpan().Verify());
}
+ TestMemory(builder.buffer());
}
}
@@ -780,6 +790,7 @@
aos::FlatbufferToJson(builder.AsFlatbufferSpan(),
{.multi_line = true}));
}
+ TestMemory(builder.buffer());
}
// Try to cover ~all supported scalar/flatbuffer types using JSON convenience
@@ -804,14 +815,14 @@
builder.get()->set_foo_float(1.111);
ASSERT_TRUE(builder.Verify());
ASSERT_FLOAT_EQ(1.111, builder.get()->AsFlatbuffer().foo_float());
+ TestMemory(builder.buffer());
}
// Confirm that we can use the SpanAllocator with a span that provides exactly
// the required buffer size.
TEST_F(StaticFlatbuffersTest, ExactSizeSpanAllocator) {
- std::vector<uint8_t> buffer;
- buffer.resize(Builder<TestTableStatic>::kBufferSize, 0);
- aos::fbs::SpanAllocator allocator({buffer.data(), buffer.size()});
+ uint8_t buffer[Builder<TestTableStatic>::kBufferSize];
+ aos::fbs::SpanAllocator allocator({buffer, sizeof(buffer)});
Builder<TestTableStatic> builder(&allocator);
TestTableStatic *object = builder.get();
object->set_scalar(123);
@@ -858,6 +869,7 @@
VLOG(1) << aos::FlatbufferToJson(builder.AsFlatbufferSpan(),
{.multi_line = true});
VLOG(1) << AnnotateBinaries(test_schema_, builder.buffer());
+ TestMemory(builder.buffer());
}
// Test that when we provide too small of a span to the Builder that it
@@ -872,9 +884,8 @@
// Verify that if we create a span with extra headroom that that lets us
// dynamically alter the size of vectors in the flatbuffers.
TEST_F(StaticFlatbuffersTest, ExtraLargeSpanAllocator) {
- std::vector<uint8_t> buffer;
- buffer.resize(Builder<TestTableStatic>::kBufferSize + 10000, 0);
- aos::fbs::SpanAllocator allocator({buffer.data(), buffer.size()});
+ uint8_t buffer[Builder<TestTableStatic>::kBufferSize + 10000];
+ aos::fbs::SpanAllocator allocator({buffer, sizeof(buffer)});
Builder<TestTableStatic> builder(&allocator);
TestTableStatic *object = builder.get();
{
@@ -893,5 +904,6 @@
*object->AsFlatbuffer().unspecified_length_vector()) {
EXPECT_EQ(expected++, value);
}
+ TestMemory(builder.buffer());
}
} // namespace aos::fbs::testing
diff --git a/aos/flatbuffers/static_vector.h b/aos/flatbuffers/static_vector.h
index 0b5d217..1752e00 100644
--- a/aos/flatbuffers/static_vector.h
+++ b/aos/flatbuffers/static_vector.h
@@ -103,6 +103,9 @@
// Note that no padding is required on the end because T::kAlign will always
// end up being equal to the alignment (this can only be violated if
// kForceAlign is used, but we do not allow that).
+// The Vector class leaves any padding uninitialized. Until and unless we
+// determine that it is a performance issue, it is the responsibility of the
+// parent of this object to zero-initialize the memory.
template <typename T, size_t kStaticLength, bool kInline,
size_t kForceAlign = 0, bool kNullTerminate = false>
class Vector : public ResizeableObject {
@@ -164,11 +167,7 @@
: ResizeableObject(buffer, parent) {
CHECK_EQ(0u, reinterpret_cast<size_t>(buffer.data()) % kAlign);
CHECK_EQ(kSize, buffer.size());
- // Set padding and length to zero.
- internal::ClearSpan(internal::GetSubSpan(buffer, 0, kPadding1));
SetLength(0u);
- internal::ClearSpan(internal::GetSubSpan(
- buffer, kPadding1 + kLengthSize + kInlineSize, kPadding2));
if (!kInline) {
// Initialize the offsets for any sub-tables. These are used to track
// where each table will get serialized in memory as memory gets