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

Allocate space for locals together with stack #382

Merged
merged 3 commits into from
Sep 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
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
14 changes: 4 additions & 10 deletions lib/fizzy/execute.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -477,10 +477,7 @@ ExecutionResult execute(
const auto& code = instance.module.codesec[code_idx];
auto* const memory = instance.memory.get();

std::vector<Value> locals(args.size() + code.local_count);
std::copy_n(args.begin(), args.size(), locals.begin());

OperandStack stack(static_cast<size_t>(code.max_stack_height));
OperandStack stack(args, code.local_count, static_cast<size_t>(code.max_stack_height));

const Instr* pc = code.instructions.data();
const uint8_t* immediates = code.immediates.data();
Expand Down Expand Up @@ -614,22 +611,19 @@ ExecutionResult execute(
case Instr::local_get:
{
const auto idx = read<uint32_t>(immediates);
assert(idx <= locals.size());
stack.push(locals[idx]);
stack.push(stack.local(idx));
break;
}
case Instr::local_set:
{
const auto idx = read<uint32_t>(immediates);
assert(idx <= locals.size());
locals[idx] = stack.pop();
stack.local(idx) = stack.pop();
break;
}
case Instr::local_tee:
{
const auto idx = read<uint32_t>(immediates);
assert(idx <= locals.size());
locals[idx] = stack.top();
stack.local(idx) = stack.top();
break;
}
case Instr::global_get:
Expand Down
92 changes: 59 additions & 33 deletions lib/fizzy/stack.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#pragma once

#include "cxx20/span.hpp"
#include "value.hpp"
#include <cassert>
#include <cstdint>
Expand Down Expand Up @@ -49,61 +50,97 @@ class Stack
}
};


/// Contains current frame's locals (including arguments) and operand stack.
/// The storage space for locals and operand stack together is allocated as a
/// continuous memory. Elements occupy the storage space in the order:
/// arguments, local variables, operand stack. Arguments and local variables can
/// be accessed under a single namespace using local() method and are separate
/// from the stack itself.
class OperandStack
{
/// The size of the pre-allocated internal storage: 128 bytes.
static constexpr auto small_storage_size = 128 / sizeof(Value);

/// The pointer to the top item, or below the stack bottom if stack is empty.
/// The pointer to the top item of the operand stack,
/// or below the stack bottom if stack is empty.
///
/// This pointer always alias m_storage, but it is kept as the first field
/// This pointer always alias m_locals, but it is kept as the first field
/// because it is accessed the most. Therefore, it must be initialized
/// in the constructor after the m_storage.
/// in the constructor after the m_locals.
Value* m_top;

/// The pointer to the beginning of the locals array.
/// This always points to one of the storages.
Value* m_locals;
Copy link
Member

Choose a reason for hiding this comment

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

Would the order of these three manifest speed differences?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have corrected this. New version may be slightly faster, but hard to tell for sure as they are very close.


/// The pointer to the bottom of the operand stack.
Value* m_bottom;

/// The pre-allocated internal storage.
Value m_small_storage[small_storage_size];

/// The unbounded storage for items.
std::unique_ptr<Value[]> m_large_storage;

Value* bottom() noexcept { return m_large_storage ? m_large_storage.get() : m_small_storage; }

const Value* bottom() const noexcept
{
return m_large_storage ? m_large_storage.get() : m_small_storage;
}

public:
/// Default constructor.
///
/// Based on @p max_stack_height decides if to use small pre-allocated storage or allocate
/// large storage.
/// Sets the top item pointer to below the stack bottom.
explicit OperandStack(size_t max_stack_height)
/// Based on required storage space decides to use small pre-allocated
/// storage or allocate large storage.
/// Sets the top stack operand pointer to below the operand stack bottom.
/// @param args Function arguments. Values are copied at the beginning of the
/// storage space.
/// @param num_local_variables The number of the function local variables (excluding
/// arguments). This number of values is zeroed in the storage space
/// after the arguments.
/// @param max_stack_height The maximum operand stack height in the function. This excludes
/// args and num_local_variables.
OperandStack(span<const Value> args, size_t num_local_variables, size_t max_stack_height)
{
if (max_stack_height > small_storage_size)
m_large_storage = std::make_unique<Value[]>(max_stack_height);
m_top = bottom() - 1;
const auto num_args = args.size();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this one could make sense to be typed as size_t because then we can be sure storage_size_required is size_t? Though I guess size() returns that anyhow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is fine now, but probably should be changed to uint32_t at some point.

const auto storage_size_required = num_args + num_local_variables + max_stack_height;

if (storage_size_required <= small_storage_size)
{
m_locals = &m_small_storage[0];
}
else
{
m_large_storage = std::make_unique<Value[]>(storage_size_required);
m_locals = &m_large_storage[0];
}

m_bottom = m_locals + num_args + num_local_variables;
m_top = m_bottom - 1;

std::copy(std::begin(args), std::end(args), m_locals);
std::fill_n(m_locals + num_args, num_local_variables, 0);
}

OperandStack(const OperandStack&) = delete;
OperandStack& operator=(const OperandStack&) = delete;

Value& local(size_t index) noexcept
{
assert(m_locals + index < m_bottom);
return m_locals[index];
}

/// The current number of items on the stack (aka stack height).
size_t size() const noexcept { return static_cast<size_t>(m_top + 1 - bottom()); }
size_t size() const noexcept { return static_cast<size_t>(m_top + 1 - m_bottom); }

/// Returns the reference to the top item.
/// Requires non-empty stack.
auto& top() noexcept
Value& top() noexcept
{
assert(size() != 0);
return *m_top;
}

/// Returns the reference to the stack item on given position from the stack top.
/// Requires index < size().
auto& operator[](size_t index) noexcept
Value& operator[](size_t index) noexcept
{
assert(index < size());
return *(m_top - index);
Expand All @@ -115,7 +152,7 @@ class OperandStack

/// Returns an item popped from the top of the stack.
/// Requires non-empty stack.
auto pop() noexcept
Value pop() noexcept
{
assert(size() != 0);
return *m_top--;
Expand All @@ -127,19 +164,8 @@ class OperandStack
m_top -= num;
}

/// Shrinks the stack to the given new size by dropping items from the top.
///
/// Requires new_size <= size().
/// shrink(0) clears entire stack and moves the top pointer below the stack base.
void shrink(size_t new_size) noexcept
{
assert(new_size <= size());
// For new_size == 0, the m_top will point below the storage.
m_top = bottom() + new_size - 1;
}

/// Returns iterator to the bottom of the stack.
const Value* rbegin() const noexcept { return bottom(); }
const Value* rbegin() const noexcept { return m_bottom; }

/// Returns end iterator counting from the bottom of the stack.
const Value* rend() const noexcept { return m_top + 1; }
Expand Down
2 changes: 1 addition & 1 deletion test/unittests/span_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ TEST(span, array)

TEST(span, stack)
{
OperandStack stack(4);
OperandStack stack({}, 0, 4);
stack.push(10);
stack.push(11);
stack.push(12);
Expand Down
Loading