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

chore: prefetch keys during transaction scheduling #4355

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
11 changes: 11 additions & 0 deletions src/core/dash.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ class DashTable : public detail::DashTableBase {
template <typename U> const_iterator Find(U&& key) const;
template <typename U> iterator Find(U&& key);

// Prefetches the memory where the key would resize into the cache.
template <typename U> void Prefetch(U&& key) const;

// Find first entry with given key hash that evaulates to true on pred.
// Pred accepts either (const key&) or (const key&, const value&)
template <typename Pred> iterator FindFirst(uint64_t key_hash, Pred&& pred);
Expand Down Expand Up @@ -699,6 +702,14 @@ auto DashTable<_Key, _Value, Policy>::Find(U&& key) -> iterator {
return FindFirst(DoHash(key), EqPred(key));
}

template <typename _Key, typename _Value, typename Policy>
template <typename U>
void DashTable<_Key, _Value, Policy>::Prefetch(U&& key) const {
uint64_t key_hash = DoHash(key);
uint32_t seg_id = SegmentId(key_hash);
segment_[seg_id]->Prefetch(key_hash);
}

template <typename _Key, typename _Value, typename Policy>
template <typename Pred>
auto DashTable<_Key, _Value, Policy>::FindFirst(uint64_t key_hash, Pred&& pred) -> iterator {
Expand Down
17 changes: 13 additions & 4 deletions src/core/dash_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,7 @@ template <typename _Key, typename _Value, typename Policy = DefaultSegmentPolicy

// Find item with given key hash and truthy predicate
template <typename Pred> Iterator FindIt(Hash_t key_hash, Pred&& pred) const;
void Prefetch(Hash_t key_hash) const;

// Returns valid iterator if succeeded or invalid if not (it's full).
// Requires: key should be not present in the segment.
Expand Down Expand Up @@ -1121,10 +1122,6 @@ auto Segment<Key, Value, Policy>::FindIt(Hash_t key_hash, Pred&& pred) const ->
uint8_t bidx = BucketIndex(key_hash);
const Bucket& target = bucket_[bidx];

// It helps a bit (10% on my home machine) and more importantly, it does not hurt
// since we are going to access this memory in a bit.
__builtin_prefetch(&target);

uint8_t fp_hash = key_hash & kFpMask;
SlotId sid = target.FindByFp(fp_hash, false, pred);
if (sid != BucketType::kNanSlot) {
Expand Down Expand Up @@ -1188,6 +1185,18 @@ auto Segment<Key, Value, Policy>::FindIt(Hash_t key_hash, Pred&& pred) const ->
return Iterator{};
}

template <typename Key, typename Value, typename Policy>
void Segment<Key, Value, Policy>::Prefetch(Hash_t key_hash) const {
uint8_t bidx = BucketIndex(key_hash);
const Bucket& target = bucket_[bidx];
uint8_t nid = NextBid(bidx);
const Bucket& probe = bucket_[nid];

// Prefetch buckets that might hold the key with high probability.
__builtin_prefetch(&target, 0, 1);
__builtin_prefetch(&probe, 0, 1);
Copy link
Contributor

@BorysTheDev BorysTheDev Dec 23, 2024

Choose a reason for hiding this comment

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

during high load this can create extra pressure on memory bandwidth, have you tested without __builtin_prefetch(&probe, 0, 1); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but we are going to look up these keys anyways

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, I have not. I will check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also check these changes on the 64 or if possible 128 core/thread CPU where memory bandwidth pressure can be higher

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the bandwidth concerns: DDR5 allows 64GB/s per DIMM: https://en.wikipedia.org/wiki/DDR5_SDRAM#:~:text=DDR5%20also%20has%20higher%20frequencies,s)%20of%20bandwidth%20per%20DIMM. 64 CPUs servers are likely to have at least 32-64 DIMMs.

Here we are talking about 1-50M/s key ops, and each prefetch loads 64 bytes. This translates to 300MB/s at top. I do not think the numbers are even close to be a concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

let me clarify my concern. The performance of DDR5 is great (as I know, the maximum number of channels is 12), but the cache size isn't so huge and the cache is not as efficient for databases as for other application types. Loading data that we need with some probability evicts other data from the cache and we need to load it again.
What I want to say is if we get the same result without loading the next bucket we don't need it to load

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 actually do not succeed to reproduce improvement at all. the difference in performance is within the statistical noise of benchmarking. I am putting this on hold.

}

template <typename Key, typename Value, typename Policy>
template <typename Cb>
void Segment<Key, Value, Policy>::TraverseAll(Cb&& cb) const {
Expand Down
42 changes: 35 additions & 7 deletions src/server/transaction.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1171,23 +1171,51 @@ void Transaction::ScheduleBatchInShard() {
ShardId sid = shard->shard_id();
auto& sq = schedule_queues[sid];

array<ScheduleContext*, 32> batch;

for (unsigned j = 0;; ++j) {
// We pull the items from the queue in a loop until we reach the stop condition.
// TODO: we may have fairness problem here, where transactions being added up all the time
// and we never break from the loop. It is possible to break early but it's not trivial
// because we must ensure that there is another ScheduleBatchInShard callback in the queue.
// Can be checked with testing sq.armed is true when j == 1.
while (true) {
ScheduleContext* item = sq.queue.Pop();
if (!item)
unsigned len = 0;

for (; len < batch.size(); ++len) {
ScheduleContext* item = sq.queue.Pop();
if (!item)
break;

batch[len] = item;
if (item->trans->IsGlobal())
continue;

auto shard_args = item->trans->GetShardArgs(sid);
// Can be empty if the transaction is not touching any keys and is
// NO_KEY_TRANSACTIONAL.
if (shard_args.Empty())
continue;
auto& db_slice = item->trans->GetDbSlice(shard->shard_id());

// We could prefetch all the keys but this is enough to test the optimization for
// single key operations.
db_slice.GetDBTable(item->trans->GetDbIndex())->prime.Prefetch(shard_args.Front());
}

if (len == 0)
break;

if (!item->trans->ScheduleInShard(shard, item->optimistic_execution)) {
item->fail_cnt.fetch_add(1, memory_order_relaxed);
stats.tx_batch_scheduled_items_total += len;

for (unsigned i = 0; i < len; ++i) {
ScheduleContext* item = batch[i];
if (!item->trans->ScheduleInShard(shard, item->optimistic_execution)) {
item->fail_cnt.fetch_add(1, memory_order_relaxed);
}
item->trans->FinishHop();
}
item->trans->FinishHop();
stats.tx_batch_scheduled_items_total++;
};
}

// j==1 means we already signalled that we're done with the current batch.
if (j == 1)
Expand Down
Loading