Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes map fields' behavior when used with arena. #459

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Member

xfxyjwf commented Jun 3, 2015

  1. Implemented NewDeleteCapture to detect heap allocations.
  2. Made MapField allocate mutexs lazily so generated API will not cause any heap allocation when using arena.
  3. Added a type trait Arena::is_destructor_skippable to check whether an object's destructor can be skipped when used with arena. Removed some incorrectly marked DestructorSkippable_ typedef.

xfxyjwf added some commits Jun 3, 2015

Fix Map/MapField's arena behavior.
Changed MapField to allocate mutex lazily and removed some incorrectly used DestructorSkippable_.

@googlebot googlebot added the cla: yes label Jun 3, 2015

Disable heap check for arena lite runtime test.
Currently the parsing code of lite messges will store unknown fields in a
string object that is not allocated in the arena. Before that bug is fixed,
heap check will fail for arena lite runtime tests.
Member

xfxyjwf commented Jun 3, 2015

Added a new commit to disable heap check for arena lite runtime tests. These tests will fail because of #445

Member

xfxyjwf commented Jun 6, 2015

@TeBoring to review

@TeBoring TeBoring commented on the diff Jul 9, 2015

src/google/protobuf/arena.h
@@ -495,6 +495,20 @@ class LIBPROTOBUF_EXPORT Arena {
static const type value;
};
+ template<typename T>
@TeBoring

TeBoring Jul 9, 2015

Contributor

We already have this internally.

@TeBoring TeBoring commented on the diff Jul 9, 2015

src/google/protobuf/map.h
@@ -439,7 +445,15 @@ class Map {
friend class ::google::protobuf::Arena;
typedef void InternalArenaConstructable_;
- typedef void DestructorSkippable_;
+ // A class is only DestructorSkippable if all of its base classes and
+ // embeded data members are DestructorSkippable.
+ //
+ // typedef typename internal::enable_if<
+ // Arena::is_destructor_skippable<
+ // hash_map<Key, value_type*, hash<Key>,
+ // equal_to<Key>, Allocator> >::value
+ // >::type DestructorSkippable_;
@TeBoring

TeBoring Jul 9, 2015

Contributor

We keep DestructorSkippable_ but let arena own the destructor of elements_.

@TeBoring TeBoring commented on the diff Jul 9, 2015

src/google/protobuf/map_field.cc
@@ -61,8 +61,30 @@ void RegisterMapEntryDefaultInstance(MessageLite* default_instance) {
map_entry_default_instances_->push_back(default_instance);
}
+
+MapFieldBase::MapFieldBase()
+ : arena_(NULL),
+ repeated_field_(NULL),
+ entry_descriptor_(NULL),
+ assign_descriptor_callback_(NULL),
+ state_(STATE_MODIFIED_MAP) {
+ lazy_mutex_ = new LazyMutex();
@TeBoring

TeBoring Jul 9, 2015

Contributor

The same as elements_ in Map?

@TeBoring TeBoring commented on the diff Jul 9, 2015

src/google/protobuf/map_field.h
@@ -213,7 +214,12 @@ class MapField : public MapFieldBase,
private:
typedef void InternalArenaConstructable_;
- typedef void DestructorSkippable_;
+ typedef typename internal::enable_if<
+ Arena::is_destructor_skippable<MapFieldBase>::value &&
+ Arena::is_destructor_skippable<
+ MapFieldLite<Key, T, kKeyFieldType, kValueFieldType,
+ default_enum_value> >::value
+ >::type DestructorSkippable_;
@TeBoring

TeBoring Jul 9, 2015

Contributor

If member is not DestructorSkippable, we can let arena own its destructor.

@TeBoring TeBoring added this to the 3.0.0-beta-1 milestone Jul 9, 2015

Contributor

TeBoring commented Jul 17, 2015

@xfxyjwf to reply new comments

Member

xfxyjwf commented Jul 17, 2015

As you have already made part of this change in our internal code base, I'll hold this PR until next down-integration when I'll merge and update the code.

@xfxyjwf xfxyjwf removed this from the 3.0.0-beta-1 milestone Aug 28, 2015

@xfxyjwf xfxyjwf closed this Jul 13, 2016

@xfxyjwf xfxyjwf deleted the xfxyjwf:skippable branch Jul 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment