Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use best-fit strategy in Arena, now O(log(n)) instead O(n) #12048

Merged
merged 2 commits into from
Mar 22, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bench/lockedpool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,4 @@ static void BenchLockedPool(benchmark::State& state)
addr.clear();
}

BENCHMARK(BenchLockedPool, 530);
BENCHMARK(BenchLockedPool, 1300);
70 changes: 45 additions & 25 deletions src/support/lockedpool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ Arena::Arena(void *base_in, size_t size_in, size_t alignment_in):
base(static_cast<char*>(base_in)), end(static_cast<char*>(base_in) + size_in), alignment(alignment_in)
{
// Start with one free chunk that covers the entire arena
chunks_free.emplace(base, size_in);
auto it = size_to_free_chunk.emplace(size_in, base);
chunks_free.emplace(base, it);
chunks_free_end.emplace(base + size_in, it);
}

Arena::~Arena()
Expand All @@ -63,26 +65,30 @@ void* Arena::alloc(size_t size)
if (size == 0)
return nullptr;

// Pick a large enough free-chunk
auto it = std::find_if(chunks_free.begin(), chunks_free.end(),
[=](const std::map<char*, size_t>::value_type& chunk){ return chunk.second >= size; });
if (it == chunks_free.end())
// Pick a large enough free-chunk. Returns an iterator pointing to the first element that is not less than key.
// This allocation strategy is best-fit. According to "Dynamic Storage Allocation: A Survey and Critical Review",
// Wilson et. al. 1995, http://www.scs.stanford.edu/14wi-cs140/sched/readings/wilson.pdf, best-fit and first-fit
// policies seem to work well in practice.
auto sizePtrIt = size_to_free_chunk.lower_bound(size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Variable names are snake_case, per developer guidelines. Fix sizePtrIt, sizeRemaining, itRemaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

if (sizePtrIt == size_to_free_chunk.end())
return nullptr;

// Create the used-chunk, taking its space from the end of the free-chunk
auto alloced = chunks_used.emplace(it->first + it->second - size, size).first;
if (!(it->second -= size))
chunks_free.erase(it);
return reinterpret_cast<void*>(alloced->first);
}

/* extend the Iterator if other begins at its end */
template <class Iterator, class Pair> bool extend(Iterator it, const Pair& other) {
if (it->first + it->second == other.first) {
it->second += other.second;
return true;
const size_t sizeRemaining = sizePtrIt->first - size;
auto alloced = chunks_used.emplace(sizePtrIt->second + sizeRemaining, size).first;
chunks_free_end.erase(sizePtrIt->second + sizePtrIt->first);
if (sizePtrIt->first == size) {
// whole chunk is used up
chunks_free.erase(sizePtrIt->second);
} else {
// still some memory left in the chunk
auto itRemaining = size_to_free_chunk.emplace(sizeRemaining, sizePtrIt->second);
chunks_free[sizePtrIt->second] = itRemaining;
chunks_free_end.emplace(sizePtrIt->second + sizeRemaining, itRemaining);
}
return false;
size_to_free_chunk.erase(sizePtrIt);

return reinterpret_cast<void*>(alloced->first);
}

void Arena::free(void *ptr)
Expand All @@ -97,16 +103,30 @@ void Arena::free(void *ptr)
if (i == chunks_used.end()) {
throw std::runtime_error("Arena: invalid or double free");
}
auto freed = *i;
std::pair<char*, size_t> freed = *i;
chunks_used.erase(i);

// Add space to free map, coalescing contiguous chunks
auto next = chunks_free.upper_bound(freed.first);
auto prev = (next == chunks_free.begin()) ? chunks_free.end() : std::prev(next);
if (prev == chunks_free.end() || !extend(prev, freed))
prev = chunks_free.emplace_hint(next, freed);
if (next != chunks_free.end() && extend(prev, *next))
// Coalesc freed with previous chunk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Coalesce

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, fixed

auto prev = chunks_free_end.find(freed.first);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the chunks_free_end map required instead of iterating one backwards from next as is done in the current code?

Copy link
Contributor Author

@martinus martinus Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cunks_free used to be an std::map that was sorted by char*. There it was possible to use std::prev to just find what memory address came before. I have replaced this sorted map with an std::unordered_map, then it's not possible any more to just find the previous address quickly within the same map. That's why I have added another map that contains the back links. So this change replaces one O(log(n)) lookup + std::prev() call with two O(1) lookups.

if (prev != chunks_free_end.end()) {
freed.first -= prev->second->first;
freed.second += prev->second->first;
size_to_free_chunk.erase(prev->second);
chunks_free_end.erase(prev);
}

// Coalesc freed with chunk after freed
auto next = chunks_free.find(freed.first + freed.second);
if (next != chunks_free.end()) {
freed.second += next->second->first;
size_to_free_chunk.erase(next->second);
chunks_free.erase(next);
}

// Add/set space with coalesced free chunk
auto it = size_to_free_chunk.emplace(freed.second, freed.first);
chunks_free[freed.first] = it;
chunks_free_end[freed.first + freed.second] = it;
}

Arena::Stats Arena::stats() const
Expand All @@ -115,7 +135,7 @@ Arena::Stats Arena::stats() const
for (const auto& chunk: chunks_used)
r.used += chunk.second;
for (const auto& chunk: chunks_free)
r.free += chunk.second;
r.free += chunk.second->first;
r.total = r.used + r.free;
return r;
}
Expand Down
19 changes: 14 additions & 5 deletions src/support/lockedpool.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <map>
#include <mutex>
#include <memory>
#include <unordered_map>

/**
* OS-dependent allocation and deallocation of locked/pinned memory pages.
Expand Down Expand Up @@ -88,11 +89,19 @@ class Arena
*/
bool addressInArena(void *ptr) const { return ptr >= base && ptr < end; }
private:
/** Map of chunk address to chunk information. This class makes use of the
* sorted order to merge previous and next chunks during deallocation.
*/
std::map<char*, size_t> chunks_free;
std::map<char*, size_t> chunks_used;
typedef std::multimap<size_t, char*> SizeToChunkSortedMap;
/** Map to enable O(log(n)) best-fit allocation, as it's sorted by size */
SizeToChunkSortedMap size_to_free_chunk;

typedef std::unordered_map<char*, SizeToChunkSortedMap::const_iterator> ChunkToSizeMap;
/** Map from begin of free chunk to its node in size_to_free_chunk */
ChunkToSizeMap chunks_free;
/** Map from end of free chunk to its node in size_to_free_chunk */
ChunkToSizeMap chunks_free_end;

/** Map from begin of used chunk to its size */
std::unordered_map<char*, size_t> chunks_used;

/** Base address of arena */
char* base;
/** End address of arena */
Expand Down