andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove extraneous call to TrackKey (#5173)

Browse Source 
Summary:
In `PessimisticTransaction::TryLock`, we were calling `TrackKey` even when assume_tracked=true, which defeats the purpose of assume_tracked. Remove this.

For keys that are already tracked, TrackKey will actually bump some counters (num_reads/num_writes) which are consumed in `TransactionBaseImpl::GetTrackedKeysSinceSavePoint`, and this is used to determine which keys were tracked since the last savepoint. I believe this functionality should still work, since I think the user should not call GetForUpdate/Put(assume_tracked=true) across savepoints, and if they do, they should not expect the Put(assume_tracked=true) to show up as a tracked key in the second savepoint.

This is another 2-3% cpu improvement.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5173

Differential Revision: D14883809

Pulled By: lth

fbshipit-source-id: 7d09f0772da422384af0519773e310c22b0cbca3
This commit is contained in:
Manuel Ung 2019-04-12 16:34:20 -07:00 committed by Facebook Github Bot
parent fe642cbee6
commit d655a3aab7
2 changed files with 24 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
include/rocksdb/utilities/transaction.h
View File

@ -293,8 +293,10 @@ class Transaction {
// functions in WriteBatch, but will also do conflict checking on the
// keys being written.
//
// assume_tracked=true expects the key be already tracked. If valid then it
// skips ValidateSnapshot. Returns error otherwise.
// assume_tracked=true expects the key be already tracked. More
// specifically, it means the the key was previous tracked in the same
// savepoint, with the same exclusive flag, and at a lower sequence number.
// If valid then it skips ValidateSnapshot. Returns error otherwise.
//
// If this Transaction was created on an OptimisticTransactionDB, these
// functions should always return Status::OK().

23
utilities/transactions/pessimistic_transaction.cc
View File

@ -628,9 +628,26 @@ Status PessimisticTransaction::TryLock(ColumnFamilyHandle* column_family,
if (s.ok()) {
// We must track all the locked keys so that we can unlock them later. If
// the key is already locked, this func will update some stats on the
// tracked key. It could also update the tracked_at_seq if it is lower than
// the existing trackey seq.
TrackKey(cfh_id, key_str, tracked_at_seq, read_only, exclusive);
// tracked key. It could also update the tracked_at_seq if it is lower
// than the existing tracked key seq. These stats are necessary for
// RollbackToSavePoint to determine whether a key can be safely removed
// from tracked_keys_. Removal can only be done if a key was only locked
// during the current savepoint.
//
// Recall that if assume_tracked is true, we assume that TrackKey has been
// called previously since the last savepoint, with the same exclusive
// setting, and at a lower sequence number, so skipping here should be
// safe.
if (!assume_tracked) {
TrackKey(cfh_id, key_str, tracked_at_seq, read_only, exclusive);
} else {
#ifndef NDEBUG
assert(tracked_keys_cf->second.count(key_str) > 0);
const auto& info = tracked_keys_cf->second.find(key_str)->second;
assert(info.seq <= tracked_at_seq);
assert(info.exclusive == exclusive);
#endif
}
}
return s;