Skip to content

Commit

Permalink
Use uintptr_t instead of uint64_t to represent address (facebookincub…
Browse files Browse the repository at this point in the history 
…ator#8778)

Summary:
In the current `MmapArena`, `uint64_t` is used to represent an
"address", which is a bit obscure. Especially when used in map, it is
difficult to distinguish whether it represents a "length" or an
"address" (pointer).

To improve readability, `uintptr_t` is used instead of `uint64_t` when
representing an "address" (pointer).

```
   std::map<uint64_t, uint64_t> freeList_;
   std::map<uint64_t, std::unordered_set<uint64_t>> freeLookup_;
```
V.S.
```
   std::map<uintptr_t, uint64_t> freeList_;
   std::map<uint64_t, std::unordered_set<uintptr_t>> freeLookup_;
```

Pull Request resolved: facebookincubator#8778

Reviewed By: Yuhta, pedroerp

Differential Revision: D54685231

Pulled By: mbasmanova

fbshipit-source-id: e10b62e851b0d48dfca8b37b3a33674b2282703d
  • Loading branch information
@lingbin @Joe-Abraham
lingbin authored and Joe-Abraham committed Jun 7, 2024
1 parent f3e1bec commit 30ca1fb
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 47 deletions.
67 changes: 35 additions & 32 deletions velox/common/memory/MmapArena.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ MmapArena::MmapArena(size_t capacityBytes) : byteSize_(capacityBytes) {
capacityBytes);
}
address_ = reinterpret_cast<uint8_t*>(ptr);
addFreeBlock(reinterpret_cast<uint64_t>(address_), byteSize_);
addFreeBlock(reinterpret_cast<uintptr_t>(address_), byteSize_);
freeBytes_ = byteSize_;
}

Expand All @@ -60,7 +60,7 @@ void* MmapArena::allocate(uint64_t bytes) {
}
bytes = roundBytes(bytes);

// First match in the list that can give this many bytes
// First match in the list that can give this many bytes.
auto lookupItr = freeLookup_.lower_bound(bytes);
if (lookupItr == freeLookup_.end()) {
VELOX_MEM_LOG_EVERY_MS(WARNING, 1000)
Expand All @@ -71,8 +71,8 @@ void* MmapArena::allocate(uint64_t bytes) {
}

freeBytes_ -= bytes;
auto address = *(lookupItr->second.begin());
auto curFreeBytes = lookupItr->first;
auto address = *(lookupItr->second.begin());
void* result = reinterpret_cast<void*>(address);
if (curFreeBytes == bytes) {
removeFreeBlock(address, curFreeBytes);
Expand All @@ -92,10 +92,10 @@ void MmapArena::free(void* address, uint64_t bytes) {
::madvise(address, bytes, MADV_DONTNEED);
freeBytes_ += bytes;

const auto curAddr = reinterpret_cast<uint64_t>(address);
const auto curAddr = reinterpret_cast<uintptr_t>(address);
auto curIter = addFreeBlock(curAddr, bytes);
auto prevIter = freeList_.end();
uint64_t prevAddr;
uintptr_t prevAddr;
uint64_t prevBytes;
bool mergePrev = false;
if (curIter != freeList_.begin()) {
Expand All @@ -106,26 +106,28 @@ void MmapArena::free(void* address, uint64_t bytes) {
VELOX_CHECK_LE(
prevEndAddr,
curAddr,
"New free node (addr:{} size:{}) overlaps with previous free node (addr:{} size:{}) in free list",
"New free block (addr:{} size:{}) overlaps with previous free block "
"(addr:{} size:{}) in free list",
curAddr,
bytes,
prevAddr,
prevBytes);
mergePrev = prevEndAddr == curAddr;
mergePrev = (prevEndAddr == curAddr);
}

auto nextItr = std::next(curIter);
uint64_t nextAddr;
auto nextIter = std::next(curIter);
uintptr_t nextAddr;
uint64_t nextBytes;
bool mergeNext = false;
if (nextItr != freeList_.end()) {
nextAddr = nextItr->first;
nextBytes = nextItr->second;
if (nextIter != freeList_.end()) {
nextAddr = nextIter->first;
nextBytes = nextIter->second;
auto curEndAddr = curAddr + bytes;
VELOX_CHECK_LE(
curEndAddr,
nextAddr,
"New free node (addr:{} size:{}) overlaps with next free node (addr:{} size:{}) in free list",
"New free block (addr:{} size:{}) overlaps with next free block "
"(addr:{} size:{}) in free list",
curAddr,
bytes,
nextAddr,
Expand All @@ -142,38 +144,39 @@ void MmapArena::free(void* address, uint64_t bytes) {
removeFromLookup(prevAddr, prevBytes);
auto newFreeSize = curAddr - prevAddr + bytes;
if (mergeNext) {
removeFreeBlock(nextItr);
removeFreeBlock(nextIter);
newFreeSize = nextAddr - prevAddr + nextBytes;
}
freeList_[prevIter->first] = newFreeSize;
freeList_[prevAddr] = newFreeSize;
freeLookup_[newFreeSize].emplace(prevAddr);
return;
}

if (mergeNext) {
VELOX_DCHECK(!mergePrev);
removeFreeBlock(nextItr);
removeFreeBlock(nextIter);
removeFromLookup(curAddr, bytes);
const auto newFreeSize = nextAddr - curAddr + nextBytes;
freeList_[curIter->first] = newFreeSize;
freeList_[curAddr] = newFreeSize;
freeLookup_[newFreeSize].emplace(curAddr);
}
}

void MmapArena::removeFromLookup(uint64_t addr, uint64_t bytes) {
void MmapArena::removeFromLookup(uintptr_t addr, uint64_t bytes) {
freeLookup_[bytes].erase(addr);
if (freeLookup_[bytes].empty()) {
freeLookup_.erase(bytes);
}
}

std::map<uint64_t, uint64_t>::iterator MmapArena::addFreeBlock(
uint64_t address,
std::map<uintptr_t, uint64_t>::iterator MmapArena::addFreeBlock(
uintptr_t address,
uint64_t bytes) {
auto insertResult = freeList_.emplace(address, bytes);
VELOX_CHECK(
insertResult.second,
"Trying to free a memory space that is already freed. Already in free list address {} size {}. Attempted to free address {} size {}",
"Trying to free a memory space that is already freed. Already in free "
"list address {} size {}. Attempted to free address {} size {}",
address,
freeList_[address],
address,
Expand All @@ -182,24 +185,24 @@ std::map<uint64_t, uint64_t>::iterator MmapArena::addFreeBlock(
return insertResult.first;
}

void MmapArena::removeFreeBlock(uint64_t addr, uint64_t bytes) {
void MmapArena::removeFreeBlock(uintptr_t addr, uint64_t bytes) {
freeList_.erase(addr);
removeFromLookup(addr, bytes);
}

void MmapArena::removeFreeBlock(std::map<uint64_t, uint64_t>::iterator& iter) {
void MmapArena::removeFreeBlock(std::map<uintptr_t, uint64_t>::iterator& iter) {
removeFromLookup(iter->first, iter->second);
freeList_.erase(iter);
}

bool MmapArena::checkConsistency() const {
uint64_t numErrors = 0;
auto arenaEndAddress = reinterpret_cast<uint64_t>(address_) + byteSize_;
auto arenaEndAddress = reinterpret_cast<uintptr_t>(address_) + byteSize_;
auto iter = freeList_.begin();
auto end = freeList_.end();
int64_t freeListTotalBytes = 0;
while (iter != end) {
// Lookup list should contain the address
// Lookup list should contain the address.
auto freeLookupIter = freeLookup_.find(iter->second);
if (freeLookupIter == freeLookup_.end() ||
freeLookupIter->second.find(iter->first) ==
Expand All @@ -211,7 +214,7 @@ bool MmapArena::checkConsistency() const {
numErrors++;
}

// Verify current free block end
// Verify current free block end.
auto blockEndAddress = iter->first + iter->second;
if (blockEndAddress > arenaEndAddress) {
LOG(WARNING)
Expand All @@ -221,7 +224,7 @@ bool MmapArena::checkConsistency() const {
numErrors++;
}

// Verify next free block not overlapping
// Verify next free block not overlapping.
auto next = std::next(iter);
if (next != end && blockEndAddress > next->first) {
LOG(WARNING)
Expand All @@ -236,7 +239,7 @@ bool MmapArena::checkConsistency() const {
iter++;
}

// Check consistency of lookup list
// Check consistency of lookup list.
int64_t freeLookupTotalBytes = 0;
for (auto iter = freeLookup_.begin(); iter != freeLookup_.end(); iter++) {
if (iter->second.empty()) {
Expand All @@ -249,7 +252,7 @@ bool MmapArena::checkConsistency() const {
freeLookupTotalBytes += (iter->first * iter->second.size());
}

// Check consistency of freeList_ and freeLookup_ in terms of bytes
// Check consistency of freeList_ and freeLookup_ in terms of bytes.
if (freeListTotalBytes != freeLookupTotalBytes ||
freeListTotalBytes != freeBytes_) {
LOG(WARNING)
Expand Down Expand Up @@ -277,7 +280,7 @@ std::string MmapArena::toString() const {
ManagedMmapArenas::ManagedMmapArenas(uint64_t singleArenaCapacity)
: singleArenaCapacity_(singleArenaCapacity) {
auto arena = std::make_shared<MmapArena>(singleArenaCapacity);
arenas_.emplace(reinterpret_cast<uint64_t>(arena->address()), arena);
arenas_.emplace(reinterpret_cast<uintptr_t>(arena->address()), arena);
currentArena_ = arena;
}

Expand All @@ -291,14 +294,14 @@ void* ManagedMmapArenas::allocate(uint64_t bytes) {
// it ever fails again then it means requested bytes is larger than a single
// MmapArena's capacity. No further attempts will happen.
auto newArena = std::make_shared<MmapArena>(singleArenaCapacity_);
arenas_.emplace(reinterpret_cast<uint64_t>(newArena->address()), newArena);
arenas_.emplace(reinterpret_cast<uintptr_t>(newArena->address()), newArena);
currentArena_ = newArena;
return currentArena_->allocate(bytes);
}

void ManagedMmapArenas::free(void* address, uint64_t bytes) {
VELOX_CHECK(!arenas_.empty());
const uint64_t addressU64 = reinterpret_cast<uint64_t>(address);
const auto addressU64 = reinterpret_cast<uintptr_t>(address);
auto iter = arenas_.lower_bound(addressU64);
if (iter == arenas_.end() || iter->first != addressU64) {
VELOX_CHECK(iter != arenas_.begin());
Expand Down
31 changes: 16 additions & 15 deletions velox/common/memory/MmapArena.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,12 @@ class MmapArena {
/// MmapArena capacity should be multiple of kMinGrainSizeBytes.
static constexpr uint64_t kMinGrainSizeBytes = 1024 * 1024; // 1M

MmapArena(size_t capacityBytes);
explicit MmapArena(size_t capacityBytes);
~MmapArena();

void* allocate(uint64_t bytes);
void free(void* address, uint64_t bytes);

void* address() const {
return reinterpret_cast<void*>(address_);
}
Expand All @@ -49,11 +50,11 @@ class MmapArena {
return byteSize_;
}

const std::map<uint64_t, uint64_t>& freeList() const {
const std::map<uintptr_t, uint64_t>& freeList() const {
return freeList_;
}

const std::map<uint64_t, std::unordered_set<uint64_t>>& freeLookup() const {
const std::map<uint64_t, std::unordered_set<uintptr_t>>& freeLookup() const {
return freeLookup_;
}

Expand All @@ -66,7 +67,7 @@ class MmapArena {
}

/// Checks internal consistency of this MmapArena. Returns true if OK. May
/// return false if there are concurrent alocations and frees during the
/// return false if there are concurrent allocations and frees during the
/// consistency check. This is a false positive but not dangerous. This is for
/// test only
bool checkConsistency() const;
Expand All @@ -91,15 +92,15 @@ class MmapArena {
// Rounds up size to the next power of 2.
static uint64_t roundBytes(uint64_t bytes);

std::map<uint64_t, uint64_t>::iterator addFreeBlock(
uint64_t addr,
std::map<uintptr_t, uint64_t>::iterator addFreeBlock(
uintptr_t addr,
uint64_t bytes);

void removeFromLookup(uint64_t addr, uint64_t bytes);
void removeFromLookup(uintptr_t addr, uint64_t bytes);

void removeFreeBlock(uint64_t addr, uint64_t bytes);
void removeFreeBlock(uintptr_t addr, uint64_t bytes);

void removeFreeBlock(std::map<uint64_t, uint64_t>::iterator& itr);
void removeFreeBlock(std::map<uintptr_t, uint64_t>::iterator& itr);

// Total capacity size of this arena.
const uint64_t byteSize_;
Expand All @@ -111,25 +112,25 @@ class MmapArena {

// A sorted list with each entry mapping from free block address to size of
// the free block
std::map<uint64_t, uint64_t> freeList_;
std::map<uintptr_t, uint64_t> freeList_;

// A sorted look up structure that stores the block size as key and a set of
// A sorted look-up structure that stores the block size as key and a set of
// addresses of that size as value.
std::map<uint64_t, std::unordered_set<uint64_t>> freeLookup_;
std::map<uint64_t, std::unordered_set<uintptr_t>> freeLookup_;
};

/// A class that manages a set of MmapArenas. It is able to adapt itself by
/// growing the number of its managed MmapArena's when extreme memory
/// fragmentation happens.
class ManagedMmapArenas {
public:
ManagedMmapArenas(uint64_t singleArenaCapacity);
explicit ManagedMmapArenas(uint64_t singleArenaCapacity);

void* allocate(uint64_t bytes);

void free(void* address, uint64_t bytes);

const std::map<uint64_t, std::shared_ptr<MmapArena>>& arenas() const {
const std::map<uintptr_t, std::shared_ptr<MmapArena>>& arenas() const {
return arenas_;
}

Expand All @@ -138,7 +139,7 @@ class ManagedMmapArenas {
const uint64_t singleArenaCapacity_;

// A sorted list of MmapArena by its initial address
std::map<uint64_t, std::shared_ptr<MmapArena>> arenas_;
std::map<uintptr_t, std::shared_ptr<MmapArena>> arenas_;

// All allocations should come from this MmapArena. When it is no longer able
// to handle allocations it will be updated to a newly created MmapArena.
Expand Down

0 comments on commit 30ca1fb

Please sign in to comment.