Skip to content

Commit

Permalink
MB-52135: Remove isStalePriv() check in StoredValue::getNext()
Browse files Browse the repository at this point in the history
During performance testing with large number of Buckets and low quota
(see CBD-4733 for details), performance profile highlights a large
amount of time spent in HashTable::pauseResumeVisit, specifically the
function itself (i.e. not calling any child functions).

90% of the samples inside HashTable::pauseResumeVisit land inside
getNext(), specifically the call to isStalePriv(). The isStalePriv
call (and resulting bit test) is likely costly to two reasons:

1. Cache-miss on reading the first (temporal) element from the
   HashTable object (we are pointer-chasing iterating the HashTable
   and hence unlikely to have this cache line in cache).

2. Requiring atomic (acquire) memory ordering on the read. As per
   cppreference.com:

    A load operation with this memory order performs the acquire
    operation on the affected memory location: no reads or writes in
    the current thread can be reordered before this load. All writes
    in other threads that release the same atomic variable are visible
    in the current thread (see Release-Acquire ordering below)

i.e. this constrains the instruction reordering the CPU is permitted
to perform.

Given the call to isStalePriv() is just a sanity check (and we've had
stale flag - added for Ephemeral for a long time) I think it's safe to
remove it now. This is unlikely to remove all the cost associated with
traversing the HashTable chains (we will still be paying the cache
miss cost), but by allowing micro-architectural optimisations to more
readily occur we should hopefully be able to hide some of that cost.

Note that we _do_ still leave the sanity check in
StoredValue::setNext() - this has not been observed to be hot in
profiles, and it's actually more important that we don't accidently
link the chains together in the wrong way.

Change-Id: Idddba064c83e9844e7f226284ab88c52cb42b54f
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/174793
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Ben Huddleston <ben.huddleston@couchbase.com>
  • Loading branch information
daverigby committed May 13, 2022
1 parent 8c3d5ee commit 7680245
Showing 1 changed file with 0 additions and 5 deletions.
5 changes: 0 additions & 5 deletions engines/ep/src/stored-value.h
Expand Up @@ -678,11 +678,6 @@ class StoredValue {
}

UniquePtr& getNext() {
if (isStalePriv()) {
throw std::logic_error(
"StoredValue::getNext: StoredValue is stale,"
"cannot get chain next value");
}
return chain_next_or_replacement;
}

Expand Down

0 comments on commit 7680245

Please sign in to comment.