Skip to content
This repository was archived by the owner on Sep 27, 2019. It is now read-only.

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

Merged
merged 18 commits into from
Jun 15, 2018
Merged

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a9847aa
Refactor of transaction_context to reduce lookups.
mbutrovich Jun 2, 2018
96431ee
Change FindValid to Find in DataTable.
mbutrovich Jun 4, 2018
a8b1b0c
Try not to look up TileGroupHeader in Abort and Commit as much.
mbutrovich Jun 5, 2018
76e0998
Change LockFreeArray::FindValid from O(n) to O(1).
mbutrovich Jun 6, 2018
e8e5de7
Refactor.
mbutrovich Jun 6, 2018
58aa8c0
Lock_free_array refactor, documentation, and added more tests.
mbutrovich Jun 11, 2018
344c914
Formatting.
mbutrovich Jun 11, 2018
8afd1b7
Fix the last_tile_group_id optimization in TOTM.
mbutrovich Jun 11, 2018
6779ba7
Fix RecordUpdate based on PR feedback.
mbutrovich Jun 12, 2018
07d75b3
Formatting.
mbutrovich Jun 12, 2018
c37e6ef
PR feedback changes.
mbutrovich Jun 12, 2018
2aed22b
More PR feedback changes.
mbutrovich Jun 12, 2018
ac0a760
Slight logic change in RecordUpdate.
mbutrovich Jun 12, 2018
ad0381d
Fix typo.
mbutrovich Jun 12, 2018
2db074c
Added is_written_ flag back in for potential future optimizations.
mbutrovich Jun 13, 2018
d5583f0
Formatting.
mbutrovich Jun 13, 2018
30fddc2
Merge branch 'master' into friday_night
tli2 Jun 14, 2018
21253c4
Merge branch 'master' into friday_night
tli2 Jun 15, 2018
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
43 changes: 16 additions & 27 deletions src/common/container/lock_free_array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@
//
//===----------------------------------------------------------------------===//

#include <memory>

#include "common/container/lock_free_array.h"
#include "common/internal_types.h"
#include "common/logger.h"
#include "common/macros.h"
#include "common/internal_types.h"

namespace peloton {

Expand All @@ -38,26 +36,24 @@ template <typename ValueType>
LOCK_FREE_ARRAY_TYPE::~LockFreeArray() { lock_free_array.clear(); }

template <typename ValueType>
bool LOCK_FREE_ARRAY_TYPE::Update(const std::size_t &offset, ValueType value) {
void LOCK_FREE_ARRAY_TYPE::Update(const std::size_t &offset,
const ValueType &value) {
LOG_TRACE("Update at %lu", offset);
PELOTON_ASSERT(lock_free_array.size() >= offset + 1);
PELOTON_ASSERT(lock_free_array.size() > offset);
lock_free_array.at(offset) = value;
return true;
}

template <typename ValueType>
bool LOCK_FREE_ARRAY_TYPE::Append(ValueType value) {
void LOCK_FREE_ARRAY_TYPE::Append(const ValueType &value) {
LOG_TRACE("Appended value.");
lock_free_array.push_back(value);
return true;
}

template <typename ValueType>
bool LOCK_FREE_ARRAY_TYPE::Erase(const std::size_t &offset,
void LOCK_FREE_ARRAY_TYPE::Erase(const std::size_t &offset,
const ValueType &invalid_value) {
LOG_TRACE("Erase at %lu", offset);
lock_free_array.at(offset) = invalid_value;
return true;
}

template <typename ValueType>
Expand All @@ -73,23 +69,12 @@ ValueType LOCK_FREE_ARRAY_TYPE::FindValid(
const std::size_t &offset, const ValueType &invalid_value) const {
LOG_TRACE("Find Valid at %lu", offset);

std::size_t valid_array_itr = 0;
std::size_t array_itr;
auto lock_free_array_offset = lock_free_array.size();
for (array_itr = 0; array_itr < lock_free_array_offset; array_itr++) {
auto value = lock_free_array.at(array_itr);
if (value != invalid_value) {
// Check offset
if (valid_array_itr == offset) {
return value;
}

// Update valid value count
valid_array_itr++;
}
ValueType value = invalid_value;
if ((lock_free_array.size() > offset)) {
value = lock_free_array.at(offset);
}

return invalid_value;
return value;
}

template <typename ValueType>
Expand All @@ -99,10 +84,14 @@ template <typename ValueType>
bool LOCK_FREE_ARRAY_TYPE::IsEmpty() const { return lock_free_array.empty(); }

template <typename ValueType>
void LOCK_FREE_ARRAY_TYPE::Clear() { lock_free_array.clear(); }
void LOCK_FREE_ARRAY_TYPE::Clear() {
// Intel docs: To free internal arrays, call shrink_to_fit() after clear().
lock_free_array.clear();
lock_free_array.shrink_to_fit();
}

template <typename ValueType>
bool LOCK_FREE_ARRAY_TYPE::Contains(const ValueType &value) {
bool LOCK_FREE_ARRAY_TYPE::Contains(const ValueType &value) const {
bool exists = false;

for (std::size_t array_itr = 0; array_itr < lock_free_array.size();
Expand Down
24 changes: 18 additions & 6 deletions src/concurrency/timestamp_ordering_transaction_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,14 +653,19 @@ ResultType TimestampOrderingTransactionManager::CommitTransaction(
// 3. install a new tuple for insert operations.
// Iterate through each item pointer in the read write set

// TODO (Pooja): This might be inefficient since we will have to get the
// tile_group_header for each entry. Check if this needs to be consolidated
oid_t last_tile_group_id = INVALID_OID;
storage::TileGroupHeader *tile_group_header = nullptr;

for (const auto &tuple_entry : rw_set) {
ItemPointer item_ptr = tuple_entry.first;
oid_t tile_group_id = item_ptr.block;
oid_t tuple_slot = item_ptr.offset;

auto tile_group_header = storage_manager->GetTileGroup(tile_group_id)->GetHeader();
if (tile_group_id != last_tile_group_id) {
tile_group_header =
storage_manager->GetTileGroup(tile_group_id)->GetHeader();
last_tile_group_id = tile_group_id;
}

if (tuple_entry.second == RWType::READ_OWN) {
// A read operation has acquired ownership but hasn't done any further
Expand Down Expand Up @@ -805,13 +810,20 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction(
}

// Iterate through each item pointer in the read write set
// TODO (Pooja): This might be inefficient since we will have to get the
// tile_group_header for each entry. Check if this needs to be consolidated

oid_t last_tile_group_id = INVALID_OID;
storage::TileGroupHeader *tile_group_header = nullptr;

for (const auto &tuple_entry : rw_set) {
ItemPointer item_ptr = tuple_entry.first;
oid_t tile_group_id = item_ptr.block;
oid_t tuple_slot = item_ptr.offset;
auto tile_group_header = storage_manager->GetTileGroup(tile_group_id)->GetHeader();

if (tile_group_id != last_tile_group_id) {
tile_group_header =
storage_manager->GetTileGroup(tile_group_id)->GetHeader();
last_tile_group_id = tile_group_id;
}

if (tuple_entry.second == RWType::READ_OWN) {
// A read operation has acquired ownership but hasn't done any further
Expand Down
90 changes: 27 additions & 63 deletions src/concurrency/transaction_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,6 @@ TransactionContext::TransactionContext(const size_t thread_id,
Init(thread_id, isolation, read_id, commit_id);
}

TransactionContext::~TransactionContext() {}

void TransactionContext::Init(const size_t thread_id,
const IsolationLevelType isolation,
const cid_t &read_id, const cid_t &commit_id) {
Expand All @@ -80,101 +78,67 @@ void TransactionContext::Init(const size_t thread_id,

isolation_level_ = isolation;

is_written_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little unsure about removing the is_written_ and insert_count_. These variables can be used to make Commits and Aborts faster. If you figure out that the is_written_ is false, you can take an entirely different code path and treat it like a READ_ONLY transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's effectively what @lmwnshn is doing in #1402.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is okay if we get rid of insert_count_ but we should definitely use is_written_. It will help with multi statement transactions that are not explicitly set to read only.


insert_count_ = 0;

gc_set_.reset(new GCSet());
gc_object_set_.reset(new GCObjectSet());
gc_set_ = std::make_shared<GCSet>();
gc_object_set_ = std::make_shared<GCObjectSet>();

on_commit_triggers_.reset();
}

RWType TransactionContext::GetRWType(const ItemPointer &location) {
RWType rw_type = RWType::INVALID;

auto rw_set_it = rw_set_.find(location);
const auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
return rw_set_it->second;
}
return rw_type;
return RWType::INVALID;
}

void TransactionContext::RecordRead(const ItemPointer &location) {

PELOTON_ASSERT(rw_set_.find(location) == rw_set_.end() ||
(rw_set_[location] != RWType::DELETE &&
rw_set_[location] != RWType::INS_DEL));
auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
UNUSED_ATTRIBUTE RWType rw_type = rw_set_it->second;
PELOTON_ASSERT(rw_type != RWType::DELETE && rw_type != RWType::INS_DEL);
return;
}
rw_set_.insert(rw_set_it, std::make_pair(location, RWType::READ));
rw_set_[location] = RWType::READ;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we have still two lookups here, one in line 99 and one in line 103. Is it even possible to do this with a single lookup? No, because some other thread could have changed the RW set between these to lines?

Copy link
Contributor Author

@mbutrovich mbutrovich Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

As best as I can tell, based on our current semantics we can't get away from two lookups on Read, or Delete.

}

void TransactionContext::RecordReadOwn(const ItemPointer &location) {
auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
RWType rw_type = rw_set_it->second;
PELOTON_ASSERT(rw_type != RWType::DELETE && rw_type != RWType::INS_DEL);
if (rw_type == RWType::READ) {
rw_set_it->second = RWType::READ_OWN;
}
} else {
rw_set_.insert(rw_set_it, std::make_pair(location, RWType::READ_OWN));
}
PELOTON_ASSERT(rw_set_.find(location) == rw_set_.end() ||
(rw_set_[location] != RWType::DELETE &&
rw_set_[location] != RWType::INS_DEL));
rw_set_[location] = RWType::READ_OWN;
}

void TransactionContext::RecordUpdate(const ItemPointer &location) {
PELOTON_ASSERT(rw_set_.find(location) == rw_set_.end() ||
(rw_set_[location] != RWType::DELETE &&
rw_set_[location] != RWType::INS_DEL));
auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
RWType rw_type = rw_set_it->second;
if (rw_type == RWType::READ || rw_type == RWType::READ_OWN) {
is_written_ = true;
if (rw_set_it->second == RWType::READ || rw_set_it->second == RWType::READ_OWN) {
rw_set_it->second = RWType::UPDATE;
} else if (rw_type == RWType::UPDATE || rw_type == RWType::INSERT) {
return;
} else {
// DELETE or INS_DELETE
PELOTON_ASSERT(false);
}
} else {
rw_set_.insert(rw_set_it, std::make_pair(location, RWType::UPDATE));
return;
}
rw_set_[location] = RWType::UPDATE;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some functionality got lost here. If rw_type == RWType::INSERT it should not be update to RWType::UPDATE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll rearrange that logic. I was trying to get away from two lookups in the case of an insert but it won't allow that.

}

void TransactionContext::RecordInsert(const ItemPointer &location) {
auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
PELOTON_ASSERT(false);
return;
}
rw_set_.insert(rw_set_it, std::make_pair(location, RWType::INSERT));
++insert_count_;
PELOTON_ASSERT(rw_set_.find(location) == rw_set_.end());
rw_set_[location] = RWType::INSERT;
}

bool TransactionContext::RecordDelete(const ItemPointer &location) {
PELOTON_ASSERT(rw_set_.find(location) == rw_set_.end() ||
(rw_set_[location] != RWType::DELETE &&
rw_set_[location] != RWType::INS_DEL));
auto rw_set_it = rw_set_.find(location);
if (rw_set_it != rw_set_.end()) {
RWType rw_type = rw_set_it->second;

if (rw_type == RWType::READ || rw_type == RWType::READ_OWN) {
rw_set_it->second = RWType::DELETE;
is_written_ = true;
return false;
} else if (rw_type == RWType::UPDATE) {
rw_set_it->second = RWType::DELETE;
return false;
} else if (rw_type == RWType::INSERT) {
rw_set_it->second = RWType::INS_DEL;
--insert_count_;
return true;
} else {
// DELETE and INS_DEL
PELOTON_ASSERT(false);
return false;
}
if (rw_set_it != rw_set_.end() && rw_set_it->second == RWType::INSERT) {
rw_set_it->second = RWType::INS_DEL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just wondering - what does happen, if the value of rw_set_it->second changes between line 130 and line 131/133. Do we still do the right thing?

Copy link
Contributor Author

@mbutrovich mbutrovich Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I suspect it's a race that isn't handled in the current semantics of our RWSet. The hope is that we'll eventually get rid of the INS_DEL type anyway when we stop reusing tuple slots.

return true;
} else {
rw_set_.insert(rw_set_it, std::make_pair(location, RWType::DELETE));
rw_set_[location] = RWType::DELETE;
return false;
}
}
Expand Down
80 changes: 57 additions & 23 deletions src/include/common/container/lock_free_array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,6 @@

#pragma once
#include "tbb/concurrent_vector.h"
#include "tbb/tbb_allocator.h"
#include <cstdlib>
#include <cstring>
#include <cstdio>
#include <array>
#include <atomic>
#include <memory>

namespace peloton {

Expand All @@ -35,33 +28,74 @@ class LockFreeArray {
LockFreeArray();
~LockFreeArray();

// Update a item
bool Update(const std::size_t &offset, ValueType value);

// Append an item
bool Append(ValueType value);

// Get a item
/**
* Assigns the provided value to the provided offset.
*
* @param offset Element offset to update
* @param value Value to be assigned
*/
void Update(const std::size_t &offset, const ValueType &value);

/**
* Appends an element to the end of the array
*
* @param value Value to be appended
*/
void Append(const ValueType &value);

/**
* Returns the element at the offset
*
* @returns Element at offset
*/
ValueType Find(const std::size_t &offset) const;

// Get a valid item
/**
* Returns the element at the offset, or invalid_value if
* the element does not exist.
*
* @param offset Element offset to access
* @param invalid_value Sentinel value to return if element
* does not exist or offset out of range
* @returns Element at offset or invalid_value
*/
ValueType FindValid(const std::size_t &offset,
const ValueType &invalid_value) const;

// Delete key from the lock_free_array
bool Erase(const std::size_t &offset, const ValueType &invalid_value);

// Returns item count in the lock_free_array
/**
* Assigns the provided invalid_value to the provided offset.
*
* @param offset Element offset to update
* @param invalid_value Invalid value to be assigned
*/
void Erase(const std::size_t &offset, const ValueType &invalid_value);

/**
*
* @return Number of elements in the underlying structure
*/
size_t GetSize() const;

// Checks if the lock_free_array is empty
/**
*
* @return True if empty, false otherwise
*/
bool IsEmpty() const;

// Clear all elements and reset them to default value
/**
* Resets the underlying data structure to have 0 elements
*/
void Clear();

// Exists ?
bool Contains(const ValueType &value);
/**
*
* Check the lock-free array for the provided value.
* O(n) time complexity.
*
* @param value value to search for
* @return True if element present, false otherwise
*/
bool Contains(const ValueType &value) const;

private:
// lock free array
Expand Down
Loading