WritePrepared Txn: Lock-free CommitMap #2861
Conversation
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| assert(0 < delta); | ||
| assert(delta < (1ul << format.COMMIT_BITS)); | ||
| rep_ = (ps << format.PAD_BITS) & ~format.COMMIT_FILTER; | ||
| rep_ = rep_ | delta; |
There was a problem hiding this comment.
isn't it simply rep_ = (ps << format.PAD_BITS) | delta
There was a problem hiding this comment.
no, the index part of the ps should be replaced by 0. Otherwise it will not receive the bits of delta in the OR operation.
| const uint64_t COMMIT_FILTER; | ||
| }; | ||
|
|
||
| struct CommitEntry64b { |
There was a problem hiding this comment.
Add some illustration of the encoding. Also what indexed_seq mean.
| uint64_t delta = rep_ & format.COMMIT_FILTER; | ||
| // zero is reserved for uninitialized entries | ||
| assert(delta < (1ul << format.COMMIT_BITS)); | ||
| if (delta == 0) { |
There was a problem hiding this comment.
check delta at the beginning?
maysamyabandeh
force-pushed
the
txn-commitmap
branch
from
a2cc168
to
d3d4330
Compare
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -91,6 +91,8 @@ Status WritePreparedTxn::CommitInternal() { | |||
| auto s = db_impl_->WriteImpl(write_options_, working_batch, nullptr, nullptr, | |||
| log_number_, disable_memtable, &seq_used); | |||
| uint64_t& commit_seq = seq_used; | |||
| // TODO(myabandeh): Reject a commit request if AddCommitted cannot encode | |||
| // commit_seq. This happens if prep_seq <<< commit_seq. | |||
There was a problem hiding this comment.
I think we should put it into old_commit_map in that case?
There was a problem hiding this comment.
this is very rare case. we just need to put a safety guard here. as long as returning error is correct no need to complicated the impl.
|
@maysamyabandeh updated the pull request - view changes - changes since last import |
|
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
We had two proposals for lock-free commit maps. This patch implements the latter one that was simpler. We can later experiment with both proposals.
In this impl each entry is an std::atomic of uint64_t, which are accessed via memory_order_acquire/release. In x86_64 arch this is compiled to simple reads and writes from memory.