Skip to content

Commit

Permalink
Reduce size of marl::ConditionVariable
Browse files Browse the repository at this point in the history
This had grown to a whopping 456 bytes on x64.
Rework the structures to reduce this down to a more reasonable 144 bytes.

Also:
* Remove the use  and include of `defer` in conditionvariable.h. We should not be including `defer.h` in any public header.
* Only allocate the list when needed. Removes unnecessary heap allocations.
  • Loading branch information
ben-clayton committed Apr 30, 2020
1 parent ffa8828 commit 3c643dd
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 29 deletions.
21 changes: 12 additions & 9 deletions include/marl/conditionvariable.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

#include "containers.h"
#include "debug.h"
#include "defer.h"
#include "memory.h"
#include "mutex.h"
#include "scheduler.h"
Expand Down Expand Up @@ -159,10 +158,10 @@ bool ConditionVariable::wait_until(
if (pred()) {
return true;
}
numWaiting++;
defer(numWaiting--);

if (auto fiber = Scheduler::Fiber::current()) {
numWaiting++;

// Currently executing on a scheduler fiber.
// Yield to let other tasks run that can unblock this fiber.
mutex.lock();
Expand All @@ -175,14 +174,18 @@ bool ConditionVariable::wait_until(
waiting.erase(it);
mutex.unlock();

numWaiting--;
return res;
} else {
// Currently running outside of the scheduler.
// Delegate to the std::condition_variable.
numWaitingOnCondition++;
defer(numWaitingOnCondition--);
return lock.wait_until(condition, timeout, pred);
}

// Currently running outside of the scheduler.
// Delegate to the std::condition_variable.
numWaiting++;
numWaitingOnCondition++;
auto res = lock.wait_until(condition, timeout, pred);
numWaitingOnCondition--;
numWaiting--;
return res;
}

} // namespace marl
Expand Down
36 changes: 26 additions & 10 deletions include/marl/containers.h
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,11 @@ class list {
list& operator=(const list&) = delete;
list& operator=(list&&) = delete;

struct AllocationChain {
Allocation allocation;
AllocationChain* next;
};

void grow(size_t count);

static void unlink(Entry* entry, Entry*& list);
Expand All @@ -300,7 +305,7 @@ class list {
Allocator* const allocator;
size_t size_ = 0;
size_t capacity = 0;
vector<Allocation, 8> allocations;
AllocationChain* allocations = nullptr;
Entry* free = nullptr;
Entry* head = nullptr;
};
Expand Down Expand Up @@ -336,17 +341,19 @@ bool list<T>::iterator::operator!=(const iterator& rhs) const {

template <typename T>
list<T>::list(Allocator* allocator /* = Allocator::Default */)
: allocator(allocator), allocations(allocator) {
grow(8);
}
: allocator(allocator) {}

template <typename T>
list<T>::~list() {
for (auto el = head; el != nullptr; el = el->next) {
el->data.~T();
}
for (auto alloc : allocations) {
allocator->free(alloc);

auto curr = allocations;
while (curr != nullptr) {
auto next = curr->next;
allocator->free(curr->allocation);
curr = next;
}
}

Expand All @@ -369,7 +376,7 @@ template <typename T>
template <typename... Args>
typename list<T>::iterator list<T>::emplace_front(Args&&... args) {
if (free == nullptr) {
grow(capacity);
grow(std::max<size_t>(capacity, 8));
}

auto entry = free;
Expand All @@ -395,9 +402,13 @@ void list<T>::erase(iterator it) {

template <typename T>
void list<T>::grow(size_t count) {
auto const entriesSize = sizeof(Entry) * count;
auto const allocChainOffset = alignUp(entriesSize, alignof(AllocationChain));
auto const allocSize = allocChainOffset + sizeof(AllocationChain);

Allocation::Request request;
request.size = sizeof(Entry) * count;
request.alignment = alignof(Entry);
request.size = allocSize;
request.alignment = std::max(alignof(Entry), alignof(AllocationChain));
request.usage = Allocation::Usage::List;
auto alloc = allocator->allocate(request);

Expand All @@ -412,7 +423,12 @@ void list<T>::grow(size_t count) {
free = entry;
}

allocations.emplace_back(std::move(alloc));
auto allocChain = reinterpret_cast<AllocationChain*>(
reinterpret_cast<uint8_t*>(alloc.ptr) + allocChainOffset);

allocChain->allocation = alloc;
allocChain->next = allocations;
allocations = allocChain;

capacity += count;
}
Expand Down
4 changes: 2 additions & 2 deletions include/marl/event.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace marl {
// Event is a synchronization primitive used to block until a signal is raised.
class Event {
public:
enum class Mode {
enum class Mode : uint8_t {
// The event signal will be automatically reset when a call to wait()
// returns.
// A single call to signal() will only unblock a single (possibly
Expand Down Expand Up @@ -115,9 +115,9 @@ class Event {

marl::mutex mutex;
ConditionVariable cv;
containers::vector<std::shared_ptr<Shared>, 1> deps;
const Mode mode;
bool signalled;
containers::vector<std::shared_ptr<Shared>, 2> deps;
};

const std::shared_ptr<Shared> shared;
Expand Down
7 changes: 6 additions & 1 deletion include/marl/memory.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,15 @@ namespace marl {
// system.
size_t pageSize();

template <typename T>
inline T alignUp(T val, T alignment) {
return alignment * ((val + alignment - 1) / alignment);
}

// Allocation holds the result of a memory allocation from an Allocator.
struct Allocation {
// Intended usage of the allocation. Used for allocation trackers.
enum class Usage {
enum class Usage : uint8_t {
Undefined = 0,
Stack, // Fiber stack
Create, // Allocator::create(), make_unique(), make_shared()
Expand Down
1 change: 1 addition & 0 deletions src/event_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#include "marl/event.h"
#include "marl/defer.h"
#include "marl/waitgroup.h"

#include "marl_test.h"
Expand Down
9 changes: 2 additions & 7 deletions src/memory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,6 @@ inline void protectPage(void* addr) {

namespace {

template <typename T>
inline T alignUp(T val, T alignment) {
return alignment * ((val + alignment - 1) / alignment);
}

// pagedMalloc() allocates size bytes of uninitialized storage with the
// specified minimum byte alignment using OS specific page mapping calls.
// If guardLow is true then reads or writes to the page below the returned
Expand Down Expand Up @@ -188,8 +183,8 @@ void pagedFree(void* ptr,
inline void* alignedMalloc(size_t alignment, size_t size) {
size_t allocSize = size + alignment + sizeof(void*);
auto allocation = malloc(allocSize);
auto aligned = reinterpret_cast<uint8_t*>(
alignUp(reinterpret_cast<uintptr_t>(allocation), alignment)); // align
auto aligned = reinterpret_cast<uint8_t*>(marl::alignUp(
reinterpret_cast<uintptr_t>(allocation), alignment)); // align
memcpy(aligned + size, &allocation, sizeof(void*)); // pointer-to-allocation
return aligned;
}
Expand Down

0 comments on commit 3c643dd

Please sign in to comment.