Refactor MapField arena destruction to avoid destroying the Map unnecessarily and make sure to destroy the mutex in MapFieldBase.

PiperOrigin-RevId: 521838091

src/google/protobuf/compiler/cpp/field_generators/map_field.cc

@@ -335,11 +335,9 @@ void MapFieldGenerator::GenerateAggregateInitializer(
 void MapFieldGenerator::GenerateDestructorCode(io::Printer* printer) const {
   Formatter format(printer, variables_);
   if (ShouldSplit(descriptor_, options_)) {
-    format("$cached_split_ptr$->$name$_.Destruct();\n");
     format("$cached_split_ptr$->$name$_.~MapField$lite$();\n");
     return;
   }
-  format("$field$.Destruct();\n");
   format("$field$.~MapField$lite$();\n");
 }
 
@@ -351,7 +349,7 @@ void MapFieldGenerator::GenerateArenaDestructorCode(
 
   Formatter format(printer, variables_);
   // _this is the object being destructed (we are inside a static method here).
-  format("_this->$field$.Destruct();\n");
+  format("_this->$field$.ArenaDestruct();\n");
 }
 
 ArenaDtorNeeds MapFieldGenerator::NeedsArenaDestructor() const {

src/google/protobuf/dynamic_message.cc

@@ -441,8 +441,8 @@ void DynamicMessage::SharedCtor(bool lock_factory) {
                     GetArenaForAllocation());
                 if (GetOwningArena() != nullptr) {
                   // Needs to destroy the mutex member.
-                  GetOwningArena()->OwnDestructor(
-                      static_cast<DynamicMapField*>(field_ptr));
+                  static_cast<DynamicMapField*>(field_ptr)->OwnMutexDestructor(
+                      *GetOwningArena());
                 }
               } else {
                 new (field_ptr) DynamicMapField(
@@ -456,8 +456,8 @@ void DynamicMessage::SharedCtor(bool lock_factory) {
                                     GetArenaForAllocation());
                 if (GetOwningArena() != nullptr) {
                   // Needs to destroy the mutex member.
-                  GetOwningArena()->OwnDestructor(
-                      static_cast<DynamicMapField*>(field_ptr));
+                  static_cast<DynamicMapField*>(field_ptr)->OwnMutexDestructor(
+                      *GetOwningArena());
                 }
               } else {
                 new (field_ptr)

src/google/protobuf/map_field.cc

@@ -43,13 +43,6 @@ namespace protobuf {
 namespace internal {
 using ::google::protobuf::internal::DownCast;
 
-void MapFieldBase::Destruct() {
-  if (arena_ == nullptr) {
-    delete repeated_field_;
-  }
-  repeated_field_ = nullptr;
-}
-
 const RepeatedPtrFieldBase& MapFieldBase::GetRepeatedField() const {
   ConstAccess();
   SyncRepeatedFieldWithMap();
@@ -226,15 +219,13 @@ DynamicMapField::DynamicMapField(const Message* default_entry, Arena* arena)
       default_entry_(default_entry) {}
 
 DynamicMapField::~DynamicMapField() {
-  if (arena_ == nullptr) {
-    // DynamicMapField owns map values. Need to delete them before clearing the
-    // map.
-    for (auto& kv : map_) {
-      kv.second.DeleteData();
-    }
-    map_.clear();
+  ABSL_DCHECK_EQ(arena_, nullptr);
+  // DynamicMapField owns map values. Need to delete them before clearing the
+  // map.
+  for (auto& kv : map_) {
+    kv.second.DeleteData();
   }
-  Destruct();
+  map_.clear();
 }
 
 int DynamicMapField::size() const { return GetMap().size(); }

src/google/protobuf/map_field.h

@@ -358,9 +358,11 @@ class PROTOBUF_EXPORT MapFieldBase : public MapFieldBaseForParse {
 
  protected:
   ~MapFieldBase() {  // "protected" stops users from deleting a `MapFieldBase *`
-    ABSL_DCHECK(repeated_field_ == nullptr);
+    ABSL_DCHECK_EQ(arena_, nullptr);
+    delete repeated_field_;
   }
-  void Destruct();
+  void ArenaDestruct() { mutex_.~Mutex(); }
+  void OwnMutexDestructor(Arena& arena) { arena.OwnDestructor(&mutex_); }
 
  public:
   // Returns reference to internal repeated field. Data written using
@@ -515,7 +517,7 @@ class TypeDefinedMapFieldBase : public MapFieldBase {
 
  protected:
   ~TypeDefinedMapFieldBase() {}
-  using MapFieldBase::Destruct;
+  using MapFieldBase::ArenaDestruct;
 
  public:
   void MapBegin(MapIterator* map_iter) const override;
@@ -575,11 +577,8 @@ class MapField : public TypeDefinedMapFieldBase<Key, T> {
   MapField() : impl_() {}
   MapField(const MapField&) = delete;
   MapField& operator=(const MapField&) = delete;
-  virtual ~MapField() {}  // Destruct() must already have been called!
-  void Destruct() {
-    impl_.Destruct();
-    TypeDefinedMapFieldBase<Key, T>::Destruct();
-  }
+  virtual ~MapField() = default;
+  void ArenaDestruct() { TypeDefinedMapFieldBase<Key, T>::ArenaDestruct(); }
 
   // This constructor is for constant initialized global instances.
   // It uses a linker initialized mutex, so it is not compatible with regular

src/google/protobuf/map_field_lite.h

@@ -34,6 +34,7 @@
 #include <type_traits>
 
 #include "google/protobuf/port.h"
+#include "absl/log/absl_check.h"
 #include "google/protobuf/io/coded_stream.h"
 #include "google/protobuf/map.h"
 #include "google/protobuf/map_entry_lite.h"
@@ -51,10 +52,6 @@ namespace google {
 namespace protobuf {
 namespace internal {
 
-#ifndef NDEBUG
-void MapFieldLiteNotDestructed(void* map_field_lite);
-#endif
-
 // This class provides access to map field using generated api. It is used for
 // internal generated message implementation only. Users should never use this
 // directly.
@@ -75,10 +72,10 @@ class MapFieldLite {
   MapFieldLite(ArenaInitialized, Arena* arena) : MapFieldLite(arena) {}
 
 #ifdef NDEBUG
-  void Destruct() { map_.~Map(); }
-  ~MapFieldLite() {}
+  ~MapFieldLite() { map_.~Map(); }
 #else
-  void Destruct() {
+  ~MapFieldLite() {
+    ABSL_DCHECK_EQ(map_.arena(), nullptr);
     // We want to destruct the map in such a way that we can verify
     // that we've done that, but also be sure that we've deallocated
     // everything (as opposed to leaving an allocation behind with no
@@ -87,11 +84,6 @@ class MapFieldLite {
     decltype(map_) swapped_map(map_.arena());
     map_.InternalSwap(&swapped_map);
   }
-  ~MapFieldLite() {
-    if (map_.arena() == nullptr && !map_.empty()) {
-      MapFieldLiteNotDestructed(this);
-    }
-  }
 #endif
   // Accessors
   const Map<Key, T>& GetMap() const { return map_; }
@@ -192,13 +184,6 @@ struct MapEntryToMapField<
       MapFieldType;
 };
 
-#ifndef NDEBUG
-inline PROTOBUF_NOINLINE void MapFieldLiteNotDestructed(void* map_field_lite) {
-  bool proper_destruct = false;
-  ABSL_CHECK(proper_destruct) << map_field_lite;
-}
-#endif
-
 }  // namespace internal
 }  // namespace protobuf
 }  // namespace google

src/google/protobuf/map_field_test.cc

@@ -55,12 +55,17 @@ namespace internal {
 using unittest::TestAllTypes;
 
 // ArenaHolder from map_test_util.h works fine for fields other than map
-// fields.  For map fields, the Destruct() call must be made before the
-// actual destructor is called.
+// fields.  For arena-owned map fields, the ArenaDestruct() call must be made
+// because the destructor will be skipped.
 template <typename MapType>
 struct ArenaDestructor : ArenaHolder<MapType> {
-  using ArenaHolder<MapType>::ArenaHolder;
-  ~ArenaDestructor() { ArenaHolder<MapType>::get()->Destruct(); }
+  ArenaDestructor(Arena* arena)
+      : ArenaHolder<MapType>(arena), owned_by_arena(arena != nullptr) {}
+  ~ArenaDestructor() {
+    if (owned_by_arena) ArenaHolder<MapType>::get()->ArenaDestruct();
+  }
+
+  bool owned_by_arena;
 };
 
 class MapFieldBaseStub : public MapFieldBase {
@@ -68,7 +73,7 @@ class MapFieldBaseStub : public MapFieldBase {
   typedef void InternalArenaConstructable_;
   typedef void DestructorSkippable_;
   MapFieldBaseStub() {}
-  virtual ~MapFieldBaseStub() { MapFieldBase::Destruct(); }
+  virtual ~MapFieldBaseStub() {}
   explicit MapFieldBaseStub(Arena* arena) : MapFieldBase(arena) {}
   void SetMapDirty() {
     state_.store(STATE_MODIFIED_MAP, std::memory_order_relaxed);