[patch] [Bug libstdc++/61667] setting max_load_factor of unordered_map cause buckets shrink

[patch] [Bug libstdc++/61667] setting max_load_factor of unordered_map cause buckets shrink

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

Hi

     I took a closer look to this PR and there is indeed a problem. My 
comment on the PR is incorrect, I thought the problem was in the rehash 
method but is it not. It is indeed wrong to shrink buckets when invoking 
max_load_factor, it reverts the effect of preallocating buckets before a 
big insertion.

     The fix is simple. A call to _M_need_rehash rather than 
_M_bkt_for_elements/_M_next_bkt automatically initialize the rehash 
policy considering the current number of elements and buckets in the 
container.

2014-08-07  François Dumont <fdumont@gcc.gnu.org>

     * include/bits/hashtable.h (_Hashtable<>::__rehash_policy): Use
     _M_need_rehash to initialize the rehash policy and check if a rehash is
     needed.
     * testsuite/23_containers/unordered_map/modifiers/61667.cc: New.

Tested under Linux x86_64.

Ok to commit ? In 4.9 branch also perhaps ?

François


[-- Attachment #2: 61667.patch --]
[-- Type: text/x-patch, Size: 2325 bytes --]

Index: include/bits/hashtable.h
===================================================================
--- include/bits/hashtable.h	(revision 213683)
+++ include/bits/hashtable.h	(working copy)
@@ -1281,10 +1281,10 @@
 	       _H1, _H2, _Hash, _RehashPolicy, _Traits>::
     __rehash_policy(const _RehashPolicy& __pol)
     {
-      size_type __n_bkt = __pol._M_bkt_for_elements(_M_element_count);
-      __n_bkt = __pol._M_next_bkt(__n_bkt);
-      if (__n_bkt != _M_bucket_count)
-	_M_rehash(__n_bkt, _M_rehash_policy._M_state());
+      auto __do_rehash =
+	__pol._M_need_rehash(_M_bucket_count, _M_element_count, 0);
+      if (__do_rehash.first)
+	_M_rehash(__do_rehash.second, _M_rehash_policy._M_state());
       _M_rehash_policy = __pol;
     }
 
Index: testsuite/23_containers/unordered_map/modifiers/61667.cc
===================================================================
--- testsuite/23_containers/unordered_map/modifiers/61667.cc	(revision 0)
+++ testsuite/23_containers/unordered_map/modifiers/61667.cc	(working copy)
@@ -0,0 +1,44 @@
+// { dg-options "-std=gnu++11" }
+
+// Copyright (C) 2011-2014 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <unordered_map>
+#include <testsuite_hooks.h>
+
+bool test __attribute__((unused)) = true;
+
+void test01()
+{
+  std::unordered_map<int, int> um(20);
+
+  std::size_t bkt_count = um.bucket_count();
+
+  um.max_load_factor(um.max_load_factor());
+
+  VERIFY( um.bucket_count() >= bkt_count );
+
+  um.max_load_factor(um.max_load_factor() * 2.f);
+
+  VERIFY( um.bucket_count() >= bkt_count );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

Re: [patch] [Bug libstdc++/61667] setting max_load_factor of unordered_map cause buckets shrink

[Jonathan Wakely]

On 07/08/14 20:56 +0200, François Dumont wrote:
>Hi
>
>    I took a closer look to this PR and there is indeed a problem. My 
>comment on the PR is incorrect, I thought the problem was in the 
>rehash method but is it not. It is indeed wrong to shrink buckets when 
>invoking max_load_factor, it reverts the effect of preallocating 
>buckets before a big insertion.
>
>    The fix is simple. A call to _M_need_rehash rather than 
>_M_bkt_for_elements/_M_next_bkt automatically initialize the rehash 
>policy considering the current number of elements and buckets in the 
>container.
>
>2014-08-07  François Dumont <fdumont@gcc.gnu.org>
>
>    * include/bits/hashtable.h (_Hashtable<>::__rehash_policy): Use
>    _M_need_rehash to initialize the rehash policy and check if a rehash is
>    needed.
>    * testsuite/23_containers/unordered_map/modifiers/61667.cc: New.
>
>Tested under Linux x86_64.
>
>Ok to commit ? In 4.9 branch also perhaps ?

OK for trunk.

If you're confident it's correct and safe then please also commit it
to the 4.9 branch - thanks.