llvm.org GIT mirror llvm / 9ed5a82
Fix DenseMap::reserve(): the formula was wrong Summary: Just running the loop in the unittests for a few more iterations (till 48) exhibit that the condition on the limit was not handled properly in r263522. Rewrite the test to use a class to count move/copies that happens when inserting into the map. Also take the opportunity to refactor the logic to compute the number of buckets required for a given number of entries in the map. Use this when constructing a DenseMap with a desired size given to the constructor (and add a tests for this). Reviewers: dblaikie Subscribers: llvm-commits Differential Revision: http://reviews.llvm.org/D18345 From: Mehdi Amini <mehdi.amini@apple.com> git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@264384 91177308-0d34-0410-b5e6-96231b3b80d8 Mehdi Amini 4 years ago
2 changed file(s) with 141 addition(s) and 20 deletion(s). Raw diff Collapse all Expand all
8080 }
8181 unsigned size() const { return getNumEntries(); }
8282
83 /// Grow the densemap so that it can contain at least Size items before
84 /// resizing again. This means somewhat more than Size buckets because
85 /// densemap resizes upon reaching 3/4 full.
86 void reserve(size_type Size) {
87 // Size *= (4/3), rounding up.
88 Size = (Size * 4 + 2) / 3;
83 /// Grow the densemap so that it can contain at least \p NumEntries items
84 /// before resizing again.
85 void reserve(size_type NumEntries) {
86 auto NumBuckets = getMinBucketToReserveForEntries(NumEntries);
8987 incrementEpoch();
90 if (Size > getNumBuckets())
91 grow(Size);
88 if (NumBuckets > getNumBuckets())
89 grow(NumBuckets);
9290 }
9391
9492 void clear() {
306304 const KeyT EmptyKey = getEmptyKey();
307305 for (BucketT *B = getBuckets(), *E = getBucketsEnd(); B != E; ++B)
308306 ::new (&B->getFirst()) KeyT(EmptyKey);
307 }
308
309 /// Returns the number of buckets to allocate to ensure that the DenseMap can
310 /// accommodate \p NumEntries without need to grow().
311 unsigned getMinBucketToReserveForEntries(unsigned NumEntries) {
312 // Ensure that "NumEntries * 4 < NumBuckets * 3"
313 if (NumEntries == 0)
314 return 0;
315 // +1 is required because of the strict equality.
316 // For example if NumEntries is 48, we need to return 401.
317 return NextPowerOf2(NumEntries * 4 / 3 + 1);
309318 }
310319
311320 void moveFromOldBuckets(BucketT *OldBucketsBegin, BucketT *OldBucketsEnd) {
585594 unsigned NumBuckets;
586595
587596 public:
588 explicit DenseMap(unsigned NumInitBuckets = 0) {
589 init(NumInitBuckets);
590 }
597 /// Create a DenseMap wth an optional \p InitialReserve that guarantee that
598 /// this number of elements can be inserted in the map without grow()
599 explicit DenseMap(unsigned InitialReserve = 0) { init(InitialReserve); }
591600
592601 DenseMap(const DenseMap &other) : BaseT() {
593602 init(0);
601610
602611 template
603612 DenseMap(const InputIt &I, const InputIt &E) {
604 init(NextPowerOf2(std::distance(I, E)));
613 init(std::distance(I, E));
605614 this->insert(I, E);
606615 }
607616
644653 }
645654 }
646655
647 void init(unsigned InitBuckets) {
656 void init(unsigned InitNumEntries) {
657 auto InitBuckets = BaseT::getMinBucketToReserveForEntries(InitNumEntries);
648658 if (allocateBuckets(InitBuckets)) {
649659 this->BaseT::initEmpty();
650660 } else {
338338 EXPECT_TRUE(cit == cit2);
339339 }
340340
341 // Make sure resize actually gives us enough buckets to insert N items
341 namespace {
342 // Simple class that counts how many moves and copy happens when growing a map
343 struct CountCopyAndMove {
344 static int Move;
345 static int Copy;
346 CountCopyAndMove() {}
347
348 CountCopyAndMove(const CountCopyAndMove &) { Copy++; }
349 CountCopyAndMove &operator=(const CountCopyAndMove &) {
350 Copy++;
351 return *this;
352 }
353 CountCopyAndMove(CountCopyAndMove &&) { Move++; }
354 CountCopyAndMove &operator=(const CountCopyAndMove &&) {
355 Move++;
356 return *this;
357 }
358 };
359 int CountCopyAndMove::Copy = 0;
360 int CountCopyAndMove::Move = 0;
361
362 } // anonymous namespace
363
364 // Test for the default minimum size of a DenseMap
365 TEST(DenseMapCustomTest, DefaultMinReservedSizeTest) {
366 // IF THIS VALUE CHANGE, please update InitialSizeTest, InitFromIterator, and
367 // ReserveTest as well!
368 const int ExpectedInitialBucketCount = 64;
369 // Formula from DenseMap::getMinBucketToReserveForEntries()
370 const int ExpectedMaxInitialEntries = ExpectedInitialBucketCount * 3 / 4 - 1;
371
372 DenseMap Map;
373 // Will allocate 64 buckets
374 Map.reserve(1);
375 unsigned MemorySize = Map.getMemorySize();
376 CountCopyAndMove::Copy = 0;
377 CountCopyAndMove::Move = 0;
378 for (int i = 0; i < ExpectedMaxInitialEntries; ++i)
379 Map.insert(std::make_pair(i, CountCopyAndMove()));
380 // Check that we didn't grow
381 EXPECT_EQ(MemorySize, Map.getMemorySize());
382 // Check that move was called the expected number of times
383 EXPECT_EQ(ExpectedMaxInitialEntries * 2, CountCopyAndMove::Move);
384 // Check that no copy occured
385 EXPECT_EQ(0, CountCopyAndMove::Copy);
386
387 // Adding one extra element should grow the map
388 CountCopyAndMove::Copy = 0;
389 CountCopyAndMove::Move = 0;
390 Map.insert(std::make_pair(ExpectedMaxInitialEntries, CountCopyAndMove()));
391 // Check that we grew
392 EXPECT_NE(MemorySize, Map.getMemorySize());
393 // Check that move was called the expected number of times
394 EXPECT_EQ(ExpectedMaxInitialEntries + 2, CountCopyAndMove::Move);
395 // Check that no copy occured
396 EXPECT_EQ(0, CountCopyAndMove::Copy);
397 }
398
399 // Make sure creating the map with an initial size of N actually gives us enough
400 // buckets to insert N items without increasing allocation size.
401 TEST(DenseMapCustomTest, InitialSizeTest) {
402 // Test a few different sizes, 48 is *not* a random choice: we need a value
403 // that is 2/3 of a power of two to stress the grow() condition, and the power
404 // of two has to be at least 64 because of minimum size allocation in the
405 // DenseMap (see DefaultMinReservedSizeTest). 66 is a value just above the
406 // 64 default init.
407 for (auto Size : {1, 2, 48, 66}) {
408 DenseMap Map(Size);
409 unsigned MemorySize = Map.getMemorySize();
410 CountCopyAndMove::Copy = 0;
411 CountCopyAndMove::Move = 0;
412 for (int i = 0; i < Size; ++i)
413 Map.insert(std::make_pair(i, CountCopyAndMove()));
414 // Check that we didn't grow
415 EXPECT_EQ(MemorySize, Map.getMemorySize());
416 // Check that move was called the expected number of times
417 EXPECT_EQ(Size * 2, CountCopyAndMove::Move);
418 // Check that no copy occured
419 EXPECT_EQ(0, CountCopyAndMove::Copy);
420 }
421 }
422
423 // Make sure creating the map with a iterator range does not trigger grow()
424 TEST(DenseMapCustomTest, InitFromIterator) {
425 std::vector> Values;
426 // The size is a random value greater than 64 (hardcoded DenseMap min init)
427 const int Count = 65;
428 for (int i = 0; i < Count; i++)
429 Values.emplace_back(i, CountCopyAndMove());
430
431 CountCopyAndMove::Move = 0;
432 CountCopyAndMove::Copy = 0;
433 DenseMap Map(Values.begin(), Values.end());
434 // Check that no move occured
435 EXPECT_EQ(0, CountCopyAndMove::Move);
436 // Check that copy was called the expected number of times
437 EXPECT_EQ(Count, CountCopyAndMove::Copy);
438 }
439
440 // Make sure reserve actually gives us enough buckets to insert N items
342441 // without increasing allocation size.
343 TEST(DenseMapCustomTest, ResizeTest) {
344 for (unsigned Size = 16; Size < 32; ++Size) {
345 DenseMap Map;
442 TEST(DenseMapCustomTest, ReserveTest) {
443 // Test a few different size, 48 is *not* a random choice: we need a value
444 // that is 2/3 of a power of two to stress the grow() condition, and the power
445 // of two has to be at least 64 because of minimum size allocation in the
446 // DenseMap (see DefaultMinReservedSizeTest). 66 is a value just above the
447 // 64 default init.
448 for (auto Size : {1, 2, 48, 66}) {
449 DenseMap Map;
346450 Map.reserve(Size);
347451 unsigned MemorySize = Map.getMemorySize();
348 for (unsigned i = 0; i < Size; ++i)
349 Map[i] = i;
350 EXPECT_TRUE(Map.getMemorySize() == MemorySize);
452 CountCopyAndMove::Copy = 0;
453 CountCopyAndMove::Move = 0;
454 for (int i = 0; i < Size; ++i)
455 Map.insert(std::make_pair(i, CountCopyAndMove()));
456 // Check that we didn't grow
457 EXPECT_EQ(MemorySize, Map.getMemorySize());
458 // Check that move was called the expected number of times
459 EXPECT_EQ(Size * 2, CountCopyAndMove::Move);
460 // Check that no copy occured
461 EXPECT_EQ(0, CountCopyAndMove::Copy);
351462 }
352463 }
353464