Make RepeatedField and RepeatedPtrField be destructor skippable for proto arenas.

This way, when e.g. `Arena::CreateMessage<RepeatedPtrField<Msg>>` is called with a non-null arena, we won't end up calling the destructor of the repeated field later.

Also add some missing includes in unittest.

PiperOrigin-RevId: 527382003
pull/12575/head
Protobuf Team Bot 2 years ago committed by Copybara-Service
parent 1b5ad0b6ad
commit e4168df564
  1. 5
      src/google/protobuf/BUILD.bazel
  2. 22
      src/google/protobuf/arena_test_util.h
  3. 1
      src/google/protobuf/map_test.cc
  4. 13
      src/google/protobuf/repeated_field.h
  5. 37
      src/google/protobuf/repeated_field_unittest.cc
  6. 3
      src/google/protobuf/repeated_ptr_field.h

@ -810,6 +810,7 @@ cc_library(
deps = [
":cc_lite_test_protos",
":test_util2",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/log:absl_check",
"@com_google_googletest//:gtest",
],
@ -1296,9 +1297,13 @@ cc_test(
}),
deps = [
":cc_test_protos",
":lite_test_util",
":protobuf",
":protobuf_lite",
"//src/google/protobuf/io",
"//src/google/protobuf/stubs",
"//src/google/protobuf/testing",
"@com_google_absl//absl/types:span",
"@com_google_googletest//:gtest",
"@com_google_googletest//:gtest_main",
],

@ -31,6 +31,9 @@
#ifndef GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__
#define GOOGLE_PROTOBUF_ARENA_TEST_UTIL_H__
#include <cstddef>
#include "absl/container/flat_hash_set.h"
#include "absl/log/absl_check.h"
#include "google/protobuf/arena.h"
#include "google/protobuf/io/coded_stream.h"
@ -84,6 +87,25 @@ struct ArenaTestPeer {
}
};
struct CleanupGrowthInfo {
size_t space_used;
absl::flat_hash_set<void*> cleanups;
};
template <typename Func>
CleanupGrowthInfo CleanupGrowth(Arena& arena, Func f) {
auto old_space_used = arena.SpaceUsed();
auto old_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena);
f();
auto new_space_used = arena.SpaceUsed();
auto new_cleanups = ArenaTestPeer::PeekCleanupListForTesting(&arena);
CleanupGrowthInfo res;
res.space_used = new_space_used - old_space_used;
res.cleanups.insert(new_cleanups.begin(), new_cleanups.end());
for (auto p : old_cleanups) res.cleanups.erase(p);
return res;
}
class NoHeapChecker {
public:
NoHeapChecker() { capture_alloc.Hook(); }

@ -28,6 +28,7 @@
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <string>
#include <type_traits>
#include <utility>

@ -137,6 +137,16 @@ void memswap(char* a, char* b) {
template <typename Element>
class RepeatedIterator;
// We can't skip the destructor for, e.g., arena allocated RepeatedField<Cord>.
template <typename Element,
bool Trivial = Arena::is_destructor_skippable<Element>::value>
struct RepeatedFieldDestructorSkippableBase {};
template <typename Element>
struct RepeatedFieldDestructorSkippableBase<Element, true> {
using DestructorSkippable_ = void;
};
} // namespace internal
// RepeatedField is used to represent repeated fields of a primitive type (in
@ -148,7 +158,8 @@ class RepeatedIterator;
// We have to specialize several methods in the Cord case to get the memory
// management right; e.g. swapping when appropriate, etc.
template <typename Element>
class RepeatedField final {
class RepeatedField final
: private internal::RepeatedFieldDestructorSkippableBase<Element> {
static_assert(
alignof(Arena) >= alignof(Element),
"We only support types that have an alignment smaller than Arena");

@ -38,6 +38,8 @@
#include "google/protobuf/repeated_field.h"
#include <algorithm>
#include <cstddef>
#include <cstdint>
#include <cstdlib>
#include <iterator>
#include <limits>
@ -45,14 +47,23 @@
#include <sstream>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
#include "google/protobuf/arena.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "absl/log/absl_check.h"
#include "absl/numeric/bits.h"
#include "absl/strings/cord.h"
#include "absl/strings/str_cat.h"
#include "absl/types/span.h"
#include "google/protobuf/arena_test_util.h"
#include "google/protobuf/io/coded_stream.h"
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
#include "google/protobuf/parse_context.h"
#include "google/protobuf/repeated_field.h"
#include "google/protobuf/repeated_ptr_field.h"
#include "google/protobuf/unittest.pb.h"
@ -1211,6 +1222,19 @@ TEST(RepeatedField, PoisonsMemoryOnAssign) {
#endif
TEST(RepeatedField, Cleanups) {
Arena arena;
auto growth = internal::CleanupGrowth(
arena, [&] { Arena::CreateMessage<RepeatedField<int>>(&arena); });
EXPECT_THAT(growth.cleanups, testing::IsEmpty());
void* ptr;
growth = internal::CleanupGrowth(arena, [&] {
ptr = Arena::CreateMessage<RepeatedField<absl::Cord>>(&arena);
});
EXPECT_THAT(growth.cleanups, testing::UnorderedElementsAre(ptr));
}
// ===================================================================
// RepeatedPtrField tests. These pretty much just mirror the RepeatedField
// tests above.
@ -1992,6 +2016,19 @@ TEST(RepeatedPtrField, DeleteSubrange) {
// DeleteSubrange is a trivial extension of ExtendSubrange.
}
TEST(RepeatedPtrField, Cleanups) {
Arena arena;
auto growth = internal::CleanupGrowth(arena, [&] {
Arena::CreateMessage<RepeatedPtrField<std::string>>(&arena);
});
EXPECT_THAT(growth.cleanups, testing::IsEmpty());
growth = internal::CleanupGrowth(arena, [&] {
Arena::CreateMessage<RepeatedPtrField<TestAllTypes>>(&arena);
});
EXPECT_THAT(growth.cleanups, testing::IsEmpty());
}
// ===================================================================
// Iterator tests stolen from net/proto/proto-array_unittest.

@ -1224,7 +1224,8 @@ class RepeatedPtrField final : private internal::RepeatedPtrFieldBase {
template <typename T>
friend struct WeakRepeatedPtrField;
typedef void InternalArenaConstructable_;
using InternalArenaConstructable_ = void;
using DestructorSkippable_ = void;
};

Loading…
Cancel
Save