Skip to content

Commit

Permalink
Fix BitUtil nextPowerOfTwo (#7531)
Browse files Browse the repository at this point in the history
Summary:
Currently `nextPowerOfTwo` uses 32-bit literal to construct `lower`, which will overflow for the input >= 2^32.

A bug introduced by this: When `AllocationPool` needs to allocation new memory from memory pool, it requests memory double the size of the previous allocation and cap at `kMaxMmapBytes` which is 512MB. But now, after some allocations, `nextPowerOfTwo` overflows and `AllocationPool` only allocates 16 huge pages.

Allocate requests' size to memory pool at `AllocationPool::newRunImpl`
```
Without fix: 64KB | 64KB | 64KB | 64KB | 32MB | 64MB | 128MB | 256MB | 512MB | 512MB |
 512MB | 512MB | 512MB | 512MB | 512MB | 512MB | 32MB | 32MB | 32MB ...

With fix:    64KB | 64KB | 64KB | 64KB | 32MB | 64MB | 128MB | 256MB | 512MB | 512MB |
 512MB | 512MB | 512MB | 512MB | 512MB | 512MB | 512MB | 512MB | 512MB ...
```

To fix, use 64-bit literal instead.

Pull Request resolved: #7531

Reviewed By: Yuhta

Differential Revision: D51524257

Pulled By: mbasmanova

fbshipit-source-id: 27b1f592396b26f04fba566b013551459b30651a
  • Loading branch information
usurai authored and facebook-github-bot committed Nov 22, 2023
1 parent 7832058 commit df984bd
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 1 deletion.
2 changes: 1 addition & 1 deletion velox/common/base/BitUtil.h
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ inline uint64_t nextPowerOfTwo(uint64_t size) {
return 0;
}
uint32_t bits = 63 - countLeadingZeros(size);
uint64_t lower = 1U << bits;
uint64_t lower = 1ULL << bits;
// Size is a power of 2.
if (lower == size) {
return size;
Expand Down
3 changes: 3 additions & 0 deletions velox/common/base/tests/BitUtilTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,9 @@ TEST_F(BitUtilTest, nextPowerOfTwo) {
EXPECT_EQ(nextPowerOfTwo(31), 32);
EXPECT_EQ(nextPowerOfTwo(32), 32);
EXPECT_EQ(nextPowerOfTwo(33), 64);
EXPECT_EQ(nextPowerOfTwo(1ULL << 32), 1ULL << 32);
EXPECT_EQ(nextPowerOfTwo((1ULL << 32) + 1), 1ULL << 33);
EXPECT_EQ(nextPowerOfTwo((1ULL << 62) + 1), 1ULL << 63);
}

TEST_F(BitUtilTest, isPowerOfTwo) {
Expand Down

0 comments on commit df984bd

Please sign in to comment.