| Message ID | c17eccff-db0f-4f0e-0f1c-45711e124ed5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix hashtable memory leak | expand |
On 24/11/18 22:58 +0100, François Dumont wrote: >Tests have shown a regression. I was building the _ReuseOrAllocNode >instance too early, node ownership was transfered but was still owned >by _Hashtable instance. > >This new version pass all tests. This is why it's worth waiting until tests have run before sending a patch for review. >Ok to commit ? OK, but please rename _M_replicate to _M_do_assign or _M_assign_impl or something that makes it clear this is part of the assignment operation. The name "replicate" isn't clear. A comment on the declaration of the new function could be helpful too. Thanks for finding this leak. I think it's worth backporting the fix, but for gcc-7-branch and gcc-8-branch it should be just: --- a/libstdc++-v3/include/bits/hashtable.h +++ b/libstdc++-v3/include/bits/hashtable.h @@ -1223,6 +1223,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION [&__roan](__node_type* __n) { return __roan(std::move_if_noexcept(__n->_M_v())); }); __ht.clear(); + if (__former_buckets) + _M_deallocate_buckets(__former_buckets, __former_bucket_count); } __catch(...) { (Plus the improvements to the tests, of course).
On 24/11/18 22:58 +0100, François Dumont wrote: >--- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc >+++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc >@@ -18,6 +18,8 @@ > // { dg-do run { target c++11 } } > > #include <unordered_set> >+#include <ext/throw_allocator.h> >+ > #include <testsuite_hooks.h> > #include <testsuite_allocator.h> > #include <testsuite_counter_type.h> >@@ -27,7 +29,9 @@ using __gnu_test::counter_type; > > void test01() > { >- typedef propagating_allocator<counter_type, false> alloc_type; >+ { >+ typedef propagating_allocator<counter_type, false, >+ __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type; You don't seem to need the throw_allocator here, can't you use __gnu_test::tracker_allocator from <testsuite_allocator.h> instead?
On 26/11/18 12:03 +0000, Jonathan Wakely wrote: >On 24/11/18 22:58 +0100, François Dumont wrote: >>Tests have shown a regression. I was building the _ReuseOrAllocNode >>instance too early, node ownership was transfered but was still >>owned by _Hashtable instance. >> >>This new version pass all tests. > >This is why it's worth waiting until tests have run before sending a >patch for review. > >>Ok to commit ? > >OK, but please rename _M_replicate to _M_do_assign or _M_assign_impl >or something that makes it clear this is part of the assignment >operation. The name "replicate" isn't clear. > >A comment on the declaration of the new function could be helpful too. > >Thanks for finding this leak. I think it's worth backporting the fix, >but for gcc-7-branch and gcc-8-branch it should be just: > >--- a/libstdc++-v3/include/bits/hashtable.h >+++ b/libstdc++-v3/include/bits/hashtable.h >@@ -1223,6 +1223,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > [&__roan](__node_type* __n) > { return __roan(std::move_if_noexcept(__n->_M_v())); }); > __ht.clear(); >+ if (__former_buckets) >+ _M_deallocate_buckets(__former_buckets, __former_bucket_count); > } > __catch(...) > { > >(Plus the improvements to the tests, of course). Because this is a regression, and we want to fix it on the branches, I've created a bugzilla to track it: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88199
I eventually called the new method _M_assign_elements. And yes, tracker_allocator was enough. Committed on trunk for the moment. François On 11/26/18 1:12 PM, Jonathan Wakely wrote: > On 24/11/18 22:58 +0100, François Dumont wrote: >> --- >> a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc >> +++ >> b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc >> @@ -18,6 +18,8 @@ >> // { dg-do run { target c++11 } } >> >> #include <unordered_set> >> +#include <ext/throw_allocator.h> >> + >> #include <testsuite_hooks.h> >> #include <testsuite_allocator.h> >> #include <testsuite_counter_type.h> >> @@ -27,7 +29,9 @@ using __gnu_test::counter_type; >> >> void test01() >> { >> - typedef propagating_allocator<counter_type, false> alloc_type; >> + { >> + typedef propagating_allocator<counter_type, false, >> + __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type; > > You don't seem to need the throw_allocator here, can't you use > __gnu_test::tracker_allocator from <testsuite_allocator.h> instead? > > . >
On 27/11/18 22:31 +0100, François Dumont wrote: >I eventually called the new method _M_assign_elements. Perfect. >And yes, tracker_allocator was enough. Great. >Committed on trunk for the moment. Great, thanks. Please note that GCC 7.4 RC1 is scheduled for this week: https://gcc.gnu.org/ml/gcc/2018-11/msg00118.html Will you be able to backport the simpler patch (without the refactoring to remove code duplication) to the branch before then? If not I can take care of it.
On 11/27/18 11:00 PM, Jonathan Wakely wrote: > On 27/11/18 22:31 +0100, François Dumont wrote: >> I eventually called the new method _M_assign_elements. > > Perfect. > >> And yes, tracker_allocator was enough. > > Great. > >> Committed on trunk for the moment. > > Great, thanks. > > Please note that GCC 7.4 RC1 is scheduled for this week: > https://gcc.gnu.org/ml/gcc/2018-11/msg00118.html > > Will you be able to backport the simpler patch (without the > refactoring to remove code duplication) to the branch before then? > > If not I can take care of it. > > > I just backport to gcc-7/8-branch the same patch. 2018-11-28 François Dumont <fdumont@gcc.gnu.org> PR libstdc++/88199 * include/bits/hashtable.h (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Deallocate former buckets after assignment. * testsuite/23_containers/unordered_set/allocator/move_assign.cc (test03): New. Bests, François Index: libstdc++-v3/include/bits/hashtable.h =================================================================== --- libstdc++-v3/include/bits/hashtable.h (révision 266538) +++ libstdc++-v3/include/bits/hashtable.h (copie de travail) @@ -1222,6 +1222,9 @@ _M_assign(__ht, [&__roan](__node_type* __n) { return __roan(std::move_if_noexcept(__n->_M_v())); }); + + if (__former_buckets) + _M_deallocate_buckets(__former_buckets, __former_bucket_count); __ht.clear(); } __catch(...) Index: libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc =================================================================== --- libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc (révision 266538) +++ libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc (copie de travail) @@ -18,6 +18,7 @@ // { dg-do run { target c++11 } } #include <unordered_set> + #include <testsuite_hooks.h> #include <testsuite_allocator.h> #include <testsuite_counter_type.h> @@ -24,10 +25,15 @@ using __gnu_test::propagating_allocator; using __gnu_test::counter_type; +using __gnu_test::tracker_allocator; +using __gnu_test::tracker_allocator_counter; void test01() { - typedef propagating_allocator<counter_type, false> alloc_type; + tracker_allocator_counter::reset(); + { + typedef propagating_allocator<counter_type, false, + tracker_allocator<counter_type>> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set<counter_type, hash, std::equal_to<counter_type>, @@ -50,9 +56,19 @@ VERIFY( counter_type::destructor_count == 2 ); } + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); +} + void test02() { - typedef propagating_allocator<counter_type, true> alloc_type; + tracker_allocator_counter::reset(); + { + typedef propagating_allocator<counter_type, true, + tracker_allocator<counter_type>> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set<counter_type, hash, std::equal_to<counter_type>, @@ -80,9 +96,55 @@ VERIFY( it == v2.begin() ); } + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); +} + +void test03() +{ + tracker_allocator_counter::reset(); + { + typedef propagating_allocator<counter_type, false, + tracker_allocator<counter_type>> alloc_type; + typedef __gnu_test::counter_type_hasher hash; + typedef std::unordered_set<counter_type, hash, + std::equal_to<counter_type>, + alloc_type> test_type; + + test_type v1(alloc_type(1)); + v1.emplace(0); + + test_type v2(alloc_type(2)); + int i = 0; + v2.emplace(i++); + for (; v2.bucket_count() == v1.bucket_count(); ++i) + v2.emplace(i); + + counter_type::reset(); + + v2 = std::move(v1); + + VERIFY( 1 == v1.get_allocator().get_personality() ); + VERIFY( 2 == v2.get_allocator().get_personality() ); + + VERIFY( counter_type::move_count == 1 ); + VERIFY( counter_type::destructor_count == i + 1 ); + } + + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); +} + int main() { test01(); test02(); + test03(); return 0; }
On 28/11/18 07:36 +0100, François Dumont wrote: >On 11/27/18 11:00 PM, Jonathan Wakely wrote: >>On 27/11/18 22:31 +0100, François Dumont wrote: >>>I eventually called the new method _M_assign_elements. >> >>Perfect. >> >>>And yes, tracker_allocator was enough. >> >>Great. >> >>>Committed on trunk for the moment. >> >>Great, thanks. >> >>Please note that GCC 7.4 RC1 is scheduled for this week: >>https://gcc.gnu.org/ml/gcc/2018-11/msg00118.html >> >>Will you be able to backport the simpler patch (without the >>refactoring to remove code duplication) to the branch before then? >> >>If not I can take care of it. >> >> >> >I just backport to gcc-7/8-branch the same patch. Thanks!
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h index efc4c4ab94f..61e177abb45 100644 --- a/libstdc++-v3/include/bits/hashtable.h +++ b/libstdc++-v3/include/bits/hashtable.h @@ -398,6 +398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_begin() const { return static_cast<__node_type*>(_M_before_begin._M_nxt); } + template<typename _Ht, typename _NodeGenerator> + void + _M_replicate(_Ht&&, const _NodeGenerator&); + template<typename _NodeGenerator> void _M_assign(const _Hashtable&, const _NodeGenerator&); @@ -1058,6 +1062,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // Reuse allocated buckets and nodes. + __reuse_or_alloc_node_type __roan(_M_begin(), *this); + _M_replicate(__ht, + [&__roan](const __node_type* __n) + { return __roan(__n->_M_v()); }); + return *this; + } + + template<typename _Key, typename _Value, + typename _Alloc, typename _ExtractKey, typename _Equal, + typename _H1, typename _H2, typename _Hash, typename _RehashPolicy, + typename _Traits> + template<typename _Ht, typename _NodeGenerator> + void + _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, + _H1, _H2, _Hash, _RehashPolicy, _Traits>:: + _M_replicate(_Ht&& __ht, const _NodeGenerator& __node_gen) + { __bucket_type* __former_buckets = nullptr; std::size_t __former_bucket_count = _M_bucket_count; const __rehash_state& __former_state = _M_rehash_policy._M_state(); @@ -1074,14 +1095,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __try { - __hashtable_base::operator=(__ht); + __hashtable_base::operator=(std::forward<_Ht>(__ht)); _M_element_count = __ht._M_element_count; _M_rehash_policy = __ht._M_rehash_policy; - __reuse_or_alloc_node_type __roan(_M_begin(), *this); _M_before_begin._M_nxt = nullptr; - _M_assign(__ht, - [&__roan](const __node_type* __n) - { return __roan(__n->_M_v()); }); + _M_assign(__ht, __node_gen); if (__former_buckets) _M_deallocate_buckets(__former_buckets, __former_bucket_count); } @@ -1099,7 +1117,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_bucket_count * sizeof(__bucket_type)); __throw_exception_again; } - return *this; } template<typename _Key, typename _Value, @@ -1227,46 +1244,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { // Can't move memory, move elements then. - __bucket_type* __former_buckets = nullptr; - size_type __former_bucket_count = _M_bucket_count; - const __rehash_state& __former_state = _M_rehash_policy._M_state(); - - if (_M_bucket_count != __ht._M_bucket_count) - { - __former_buckets = _M_buckets; - _M_buckets = _M_allocate_buckets(__ht._M_bucket_count); - _M_bucket_count = __ht._M_bucket_count; - } - else - __builtin_memset(_M_buckets, 0, - _M_bucket_count * sizeof(__bucket_type)); - - __try - { - __hashtable_base::operator=(std::move(__ht)); - _M_element_count = __ht._M_element_count; - _M_rehash_policy = __ht._M_rehash_policy; __reuse_or_alloc_node_type __roan(_M_begin(), *this); - _M_before_begin._M_nxt = nullptr; - _M_assign(__ht, + _M_replicate(std::move(__ht), [&__roan](__node_type* __n) { return __roan(std::move_if_noexcept(__n->_M_v())); }); __ht.clear(); } - __catch(...) - { - if (__former_buckets) - { - _M_deallocate_buckets(); - _M_rehash_policy._M_reset(__former_state); - _M_buckets = __former_buckets; - _M_bucket_count = __former_bucket_count; - } - __builtin_memset(_M_buckets, 0, - _M_bucket_count * sizeof(__bucket_type)); - __throw_exception_again; - } - } } template<typename _Key, typename _Value, diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc index ef6c0deb1af..59153001d29 100644 --- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc +++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc @@ -18,6 +18,8 @@ // { dg-do run { target c++11 } } #include <unordered_set> +#include <ext/throw_allocator.h> + #include <testsuite_hooks.h> #include <testsuite_allocator.h> #include <testsuite_counter_type.h> @@ -27,7 +29,9 @@ using __gnu_test::counter_type; void test01() { - typedef propagating_allocator<counter_type, false> alloc_type; + { + typedef propagating_allocator<counter_type, false, + __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set<counter_type, hash, std::equal_to<counter_type>, @@ -50,9 +54,15 @@ void test01() VERIFY( counter_type::destructor_count == 2 ); } + // Check there's nothing left allocated or constructed. + __gnu_cxx::annotate_base::check(); +} + void test02() { - typedef propagating_allocator<counter_type, true> alloc_type; + { + typedef propagating_allocator<counter_type, true, + __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set<counter_type, hash, std::equal_to<counter_type>, @@ -80,9 +90,48 @@ void test02() VERIFY( it == v2.begin() ); } + // Check there's nothing left allocated or constructed. + __gnu_cxx::annotate_base::check(); +} + +void test03() +{ + { + typedef propagating_allocator<counter_type, false, + __gnu_cxx::throw_allocator_limit<counter_type>> alloc_type; + typedef __gnu_test::counter_type_hasher hash; + typedef std::unordered_set<counter_type, hash, + std::equal_to<counter_type>, + alloc_type> test_type; + + test_type v1(alloc_type(1)); + v1.emplace(0); + + test_type v2(alloc_type(2)); + int i = 0; + v2.emplace(i++); + for (; v2.bucket_count() == v1.bucket_count(); ++i) + v2.emplace(i); + + counter_type::reset(); + + v2 = std::move(v1); + + VERIFY( 1 == v1.get_allocator().get_personality() ); + VERIFY( 2 == v2.get_allocator().get_personality() ); + + VERIFY( counter_type::move_count == 1 ); + VERIFY( counter_type::destructor_count == i + 1 ); + } + + // Check there's nothing left allocated or constructed. + __gnu_cxx::annotate_base::check(); +} + int main() { test01(); test02(); + test03(); return 0; }