<div dir="ltr">I think someone mentioned on IRC, might be worth renaming 'resize' to 'reserve' as well, to better match the naming/behavior of standard containers?</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 14, 2016 at 6:50 PM, Fiona Glaser via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: escha<br>
Date: Mon Mar 14 20:50:46 2016<br>
New Revision: 263522<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263522&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=263522&view=rev</a><br>
Log:<br>
DenseMap: make .resize() do the intuitive thing<br>
<br>
In some places, like InstCombine, we resize a DenseMap to fit the elements<br>
we intend to put in it, then insert those elements (to avoid continual<br>
reallocations as it grows). But .resize(foo) doesn't actually do what<br>
people think; it resizes to foo buckets (which is really an<br>
implementation detail the user of DenseMap probably shouldn't care about),<br>
not the space required to fit foo elements. DenseMap grows if 3/4 of its<br>
buckets are full, so this actually causes one forced reallocation every<br>
time instead of avoiding a reallocation.<br>
<br>
This patch makes .resize(foo) do the intuitive thing: it grows to the size<br>
necessary to fit foo elements without new allocations.<br>
<br>
Also include a test to verify that .resize() actually does what we think it<br>
does.<br>
<br>
Modified:<br>
    llvm/trunk/include/llvm/ADT/DenseMap.h<br>
    llvm/trunk/unittests/ADT/DenseMapTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/DenseMap.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseMap.h?rev=263522&r1=263521&r2=263522&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseMap.h?rev=263522&r1=263521&r2=263522&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ADT/DenseMap.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/DenseMap.h Mon Mar 14 20:50:46 2016<br>
@@ -81,8 +81,12 @@ public:<br>
   }<br>
   unsigned size() const { return getNumEntries(); }<br>
<br>
-  /// Grow the densemap so that it has at least Size buckets. Does not shrink<br>
+  /// Grow the densemap so that it can contain at least Size items before<br>
+  /// resizing again. This means somewhat more than Size buckets because<br>
+  /// densemap resizes upon reaching 3/4 full.<br>
   void resize(size_type Size) {<br>
+    // Size *= (4/3), rounding up.<br>
+    Size = (Size * 4 + 2) / 3;<br>
     incrementEpoch();<br>
     if (Size > getNumBuckets())<br>
       grow(Size);<br>
<br>
Modified: llvm/trunk/unittests/ADT/DenseMapTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/DenseMapTest.cpp?rev=263522&r1=263521&r2=263522&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/DenseMapTest.cpp?rev=263522&r1=263521&r2=263522&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/ADT/DenseMapTest.cpp (original)<br>
+++ llvm/trunk/unittests/ADT/DenseMapTest.cpp Mon Mar 14 20:50:46 2016<br>
@@ -339,6 +339,19 @@ TYPED_TEST(DenseMapTest, ConstIteratorTe<br>
   EXPECT_TRUE(cit == cit2);<br>
 }<br>
<br>
+// Make sure resize actually gives us enough buckets to insert N items<br>
+// without increasing allocation size.<br>
+TEST(DenseMapCustomTest, ResizeTest) {<br>
+  for (unsigned Size = 16; Size < 32; ++Size) {<br>
+    DenseMap<unsigned, unsigned> Map;<br>
+    Map.resize(Size);<br>
+    unsigned MemorySize = Map.getMemorySize();<br>
+    for (unsigned i = 0; i < Size; ++i)<br>
+      Map[i] = i;<br>
+    EXPECT_TRUE(Map.getMemorySize() == MemorySize);<br>
+  }<br>
+}<br>
+<br>
 // Make sure DenseMap works with StringRef keys.<br>
 TEST(DenseMapCustomTest, StringRefTest) {<br>
   DenseMap<StringRef, int> M;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>