* Fix hashtable memory leak
@ 2018-11-19 21:23 François Dumont
2018-11-24 21:58 ` François Dumont
0 siblings, 1 reply; 9+ messages in thread
From: François Dumont @ 2018-11-19 21:23 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 699 bytes --]
There a memory leak when move assigning between 2 instances with
different bucket count and non-propagating and unequal allocator
instances (see new test03).
I reduced code duplication to fix the issue as 1 of the 2
implementations was fine.
   * include/bits/hashtable.h (_Hashtable<>::_M_replicate): New.
   (_Hashtable<>::operator=(const _Hashtable&)): Use latter.
   (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise.
   * testsuite/23_containers/unordered_set/allocator/move_assign.cc
   (test03): New.
I still need to run all tests but new move_assign.cc works fine.
Ok to commit once fully tested ?
François
[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch, Size: 6510 bytes --]
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;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-19 21:23 Fix hashtable memory leak François Dumont
@ 2018-11-24 21:58 ` François Dumont
2018-11-26 12:03 ` Jonathan Wakely
2018-11-26 12:12 ` Jonathan Wakely
0 siblings, 2 replies; 9+ messages in thread
From: François Dumont @ 2018-11-24 21:58 UTC (permalink / raw)
To: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
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.
Ok to commit ?
François
On 11/19/18 10:23 PM, François Dumont wrote:
> There a memory leak when move assigning between 2 instances with
> different bucket count and non-propagating and unequal allocator
> instances (see new test03).
>
> I reduced code duplication to fix the issue as 1 of the 2
> implementations was fine.
>
> Â Â Â * include/bits/hashtable.h (_Hashtable<>::_M_replicate): New.
> Â Â Â (_Hashtable<>::operator=(const _Hashtable&)): Use latter.
> Â Â Â (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise.
> Â Â Â * testsuite/23_containers/unordered_set/allocator/move_assign.cc
> Â Â Â (test03): New.
>
> I still need to run all tests but new move_assign.cc works fine.
>
> Ok to commit once fully tested ?
>
> François
>
[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch, Size: 6598 bytes --]
diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h
index efc4c4ab94f..caa4e8ad20c 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,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
// Reuse allocated buckets and nodes.
+ _M_replicate(__ht,
+ [](const __reuse_or_alloc_node_type& __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 +1094,14 @@ _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()); });
+ [&__node_gen, &__roan](__node_type* __n)
+ { return __node_gen(__roan, __n); });
if (__former_buckets)
_M_deallocate_buckets(__former_buckets, __former_bucket_count);
}
@@ -1099,7 +1119,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_M_bucket_count * sizeof(__bucket_type));
__throw_exception_again;
}
- return *this;
}
template<typename _Key, typename _Value,
@@ -1227,46 +1246,11 @@ _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,
- [&__roan](__node_type* __n)
+ _M_replicate(std::move(__ht),
+ [](const __reuse_or_alloc_node_type& __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;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-24 21:58 ` François Dumont
@ 2018-11-26 12:03 ` Jonathan Wakely
2018-11-26 12:35 ` Jonathan Wakely
2018-11-26 12:12 ` Jonathan Wakely
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2018-11-26 12:03 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-26 12:03 ` Jonathan Wakely
@ 2018-11-26 12:35 ` Jonathan Wakely
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2018-11-26 12:35 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-24 21:58 ` François Dumont
2018-11-26 12:03 ` Jonathan Wakely
@ 2018-11-26 12:12 ` Jonathan Wakely
2018-11-27 21:31 ` François Dumont
1 sibling, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2018-11-26 12:12 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-26 12:12 ` Jonathan Wakely
@ 2018-11-27 21:31 ` François Dumont
2018-11-27 22:00 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: François Dumont @ 2018-11-27 21:31 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
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?
>
> .
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-27 21:31 ` François Dumont
@ 2018-11-27 22:00 ` Jonathan Wakely
2018-11-28 6:36 ` François Dumont
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Wakely @ 2018-11-27 22:00 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-27 22:00 ` Jonathan Wakely
@ 2018-11-28 6:36 ` François Dumont
2018-11-28 10:42 ` Jonathan Wakely
0 siblings, 1 reply; 9+ messages in thread
From: François Dumont @ 2018-11-28 6:36 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: libstdc++, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: hashtable_mem_leak.patch --]
[-- Type: text/x-patch; name="hashtable_mem_leak.patch", Size: 3941 bytes --]
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;
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Fix hashtable memory leak
2018-11-28 6:36 ` François Dumont
@ 2018-11-28 10:42 ` Jonathan Wakely
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Wakely @ 2018-11-28 10:42 UTC (permalink / raw)
To: François Dumont; +Cc: libstdc++, gcc-patches
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-11-28 10:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 21:23 Fix hashtable memory leak François Dumont
2018-11-24 21:58 ` François Dumont
2018-11-26 12:03 ` Jonathan Wakely
2018-11-26 12:35 ` Jonathan Wakely
2018-11-26 12:12 ` Jonathan Wakely
2018-11-27 21:31 ` François Dumont
2018-11-27 22:00 ` Jonathan Wakely
2018-11-28 6:36 ` François Dumont
2018-11-28 10:42 ` Jonathan Wakely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).