diff mbox

Simplify allocator use

Message ID 20140626113110.GG2711@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely June 26, 2014, 11:31 a.m. UTC 
On 25/06/14 21:56 +0100, Jonathan Wakely wrote:
>The other adds an RAII type to help manage pointers obtained from
>allocators. The new type means I can remove several ugly try-catch
>blocks that are all very similar in structure and have been bothering
>me for some time. The new type also makes it trivial to support
>allocators with fancy pointers, fixing long-standing (but not very
>important) bugs in std::promise and std::shared_ptr.

This patch applies the __allocated_ptr type to hashtable_policy.h to
remove most explicit deallocation (yay!)  The buckets are still
allocated and deallocated manually, because __allocated_ptr only works
for allocations of single objects, not arrays.

As well as __allocated_ptr this change relies on two things:

1) the node type has a trivial destructor, so we don't actually need
   to call it, we can just reuse or release its storage.
   (See 3.8 [basic.life] p1)

2) allocator_traits::construct and allocator_traits::destroy can be
   used with an allocator that has a different value_type, so we don't
   need to create a rebound copy to destroy every element, we can just
   use the node-allocator.
   (See http://cplusplus.github.io/LWG/lwg-active.html#2218 which is
   Open, but I've discussed the issue with Howard, Pablo and others,
   and I think libc++ already relies on this assumption).

François, could you check it, and let me know if you see anything
wrong or have any comments?

Comments

Jonathan Wakely June 26, 2014, 11:35 a.m. UTC | #1 
On 26/06/14 12:31 +0100, Jonathan Wakely wrote:
>@@ -137,20 +139,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      __node_type* __node = _M_nodes;
> 	      _M_nodes = _M_nodes->_M_next();
> 	      __node->_M_nxt = nullptr;
>-	      __value_alloc_type __a(_M_h._M_node_allocator());
>-	      __value_alloc_traits::destroy(__a, __node->_M_valptr());
>-	      __try
>-		{
>-		  __value_alloc_traits::construct(__a, __node->_M_valptr(),
>-						  std::forward<_Arg>(__arg));
>-		}
>-	      __catch(...)
>-		{
>-		  __node->~__node_type();
>-		  __node_alloc_traits::deallocate(_M_h._M_node_allocator(),
>-						  __node, 1);

I forgot to mention the change also fixes a bug in the line above:
__node should be converted to the allocator's pointer type before
passing it to deallocate. Using __allocated_ptr takes care of that.
François Dumont June 27, 2014, 9:29 p.m. UTC | #2 
On 26/06/2014 13:31, Jonathan Wakely wrote:
> On 25/06/14 21:56 +0100, Jonathan Wakely wrote:
>> The other adds an RAII type to help manage pointers obtained from
>> allocators. The new type means I can remove several ugly try-catch
>> blocks that are all very similar in structure and have been bothering
>> me for some time. The new type also makes it trivial to support
>> allocators with fancy pointers, fixing long-standing (but not very
>> important) bugs in std::promise and std::shared_ptr.
>
> This patch applies the __allocated_ptr type to hashtable_policy.h to
> remove most explicit deallocation (yay!)  The buckets are still
> allocated and deallocated manually, because __allocated_ptr only works
> for allocations of single objects, not arrays.
>
> As well as __allocated_ptr this change relies on two things:
>
> 1) the node type has a trivial destructor, so we don't actually need
>   to call it, we can just reuse or release its storage.
>   (See 3.8 [basic.life] p1)
>
> 2) allocator_traits::construct and allocator_traits::destroy can be
>   used with an allocator that has a different value_type, so we don't
>   need to create a rebound copy to destroy every element, we can just
>   use the node-allocator.
>   (See http://cplusplus.github.io/LWG/lwg-active.html#2218 which is
>   Open, but I've discussed the issue with Howard, Pablo and others,
>   and I think libc++ already relies on this assumption).
>
> François, could you check it, and let me know if you see anything
> wrong or have any comments?
>
That looks fine to me, nice simplification.

François
diff mbox

Patch

commit d2fd02daab715c79c766bc0a476d1d01da1fc305
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jun 26 12:28:56 2014 +0100

    	* include/bits/hashtable_policy.h (_ReuseOrAllocNode::operator()): Use
    	__allocated_ptr.
    	(_Hashtable_alloc::_M_allocate_node): Likewise.
    	(_Hashtable_alloc::_M_deallocate_node): Likewise.

diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h
index 606fbab..ed6b2d7 100644
--- a/libstdc++-v3/include/bits/hashtable_policy.h
+++ b/libstdc++-v3/include/bits/hashtable_policy.h
@@ -31,6 +31,8 @@ 
 #ifndef _HASHTABLE_POLICY_H
 #define _HASHTABLE_POLICY_H 1
 
+#include <bits/allocated_ptr.h>
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -137,20 +139,12 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      __node_type* __node = _M_nodes;
 	      _M_nodes = _M_nodes->_M_next();
 	      __node->_M_nxt = nullptr;
-	      __value_alloc_type __a(_M_h._M_node_allocator());
-	      __value_alloc_traits::destroy(__a, __node->_M_valptr());
-	      __try
-		{
-		  __value_alloc_traits::construct(__a, __node->_M_valptr(),
-						  std::forward<_Arg>(__arg));
-		}
-	      __catch(...)
-		{
-		  __node->~__node_type();
-		  __node_alloc_traits::deallocate(_M_h._M_node_allocator(),
-						  __node, 1);
-		  __throw_exception_again;
-		}
+	      auto& __a = _M_h._M_node_allocator();
+	      __node_alloc_traits::destroy(__a, __node->_M_valptr());
+	      __allocated_ptr<_NodeAlloc> __guard{__a, __node};
+	      __node_alloc_traits::construct(__a, __node->_M_valptr(),
+					     std::forward<_Arg>(__arg));
+	      __guard = nullptr;
 	      return __node;
 	    }
 	  return _M_h._M_allocate_node(std::forward<_Arg>(__arg));
@@ -1947,33 +1941,25 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typename _Hashtable_alloc<_NodeAlloc>::__node_type*
       _Hashtable_alloc<_NodeAlloc>::_M_allocate_node(_Args&&... __args)
       {
-	auto __nptr = __node_alloc_traits::allocate(_M_node_allocator(), 1);
-	__node_type* __n = std::__addressof(*__nptr);
-	__try
-	  {
-	    __value_alloc_type __a(_M_node_allocator());
-	    ::new ((void*)__n) __node_type;
-	    __value_alloc_traits::construct(__a, __n->_M_valptr(),
-					    std::forward<_Args>(__args)...);
-	    return __n;
-	  }
-	__catch(...)
-	  {
-	    __node_alloc_traits::deallocate(_M_node_allocator(), __nptr, 1);
-	    __throw_exception_again;
-	  }
+	auto& __a = _M_node_allocator();
+	auto __guard = std::__allocate_guarded(__a);
+	__node_type* __n = __guard.get();
+	::new ((void*)__n) __node_type;
+	__node_alloc_traits::construct(__a, __n->_M_valptr(),
+				       std::forward<_Args>(__args)...);
+	__guard = nullptr;
+	return __n;
       }
 
   template<typename _NodeAlloc>
     void
     _Hashtable_alloc<_NodeAlloc>::_M_deallocate_node(__node_type* __n)
     {
-      typedef typename __node_alloc_traits::pointer _Ptr;
-      auto __ptr = std::pointer_traits<_Ptr>::pointer_to(*__n);
-      __value_alloc_type __a(_M_node_allocator());
-      __value_alloc_traits::destroy(__a, __n->_M_valptr());
-      __n->~__node_type();
-      __node_alloc_traits::deallocate(_M_node_allocator(), __ptr, 1);
+      static_assert(std::is_trivially_destructible<__node_type>::value,
+		    "Nodes must not require non-trivial destruction");
+      auto& __alloc = _M_node_allocator();
+      __allocated_ptr<__node_alloc_type> __guard{__alloc, __n};
+      __node_alloc_traits::destroy(__alloc, __n->_M_valptr());
     }
 
   template<typename _NodeAlloc>