-
Notifications
You must be signed in to change notification settings - Fork 919
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
feat(transaction): Use single hop in squashing when possible #2376
Conversation
d3374b9
to
e704c04
Compare
src/server/multi_command_squasher.cc
Outdated
// If all commands fit into a single batch, run them as a real single hop without multi overhead. | ||
// Doesn't work with replication, so disallow if inline scheduling is not allowed. | ||
bool singlehop_possible = IsAtomic() && !tx->IsScheduled() && cmds_.empty(); | ||
if (singlehop_possible && ServerState::tlocal()->AllowInlineScheduling()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not safe obviously... making this stuff work with replication is a little cumbersome
There are two options to running a single hop: either keep the base tx multi or not. With single-shard EVAL we decided to reduce it to a regular transaction, which could have even been done much easier by just removing the multi flag after proper initialization. With MULTI/EXEC we have to handle replication, mainly sending the closing EXEC journal message with shard count, etc. If we decide to stick to non-multi single hops, we have to emulate it ourselves. Instead, I changed the logic, so that if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, a few clarification questions :)
src/server/transaction.h
Outdated
@@ -209,7 +209,8 @@ class Transaction { | |||
void StartMultiGlobal(DbIndex dbid); | |||
|
|||
// Start multi in LOCK_AHEAD mode with given keys. | |||
void StartMultiLockedAhead(DbIndex dbid, CmdArgList keys); | |||
// Scheduling can be optionally disabled to allow more fine-grained control. | |||
void StartMultiLockedAhead(DbIndex dbid, CmdArgList keys, bool skip_scheduling = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use an enum instead of boolean argument here. kSkipScheduling at the call site is more readable and less error prone (for example, I'd assume that the meaning would be schedule
rather than skip_scheduling
)
src/server/main_service.cc
Outdated
@@ -1721,7 +1721,7 @@ optional<bool> StartMultiEval(DbIndex dbid, CmdArgList keys, ScriptMgr::ScriptPa | |||
trans->StartMultiGlobal(dbid); | |||
return true; | |||
case Transaction::LOCK_AHEAD: | |||
trans->StartMultiLockedAhead(dbid, keys); | |||
trans->StartMultiLockedAhead(dbid, keys, true); | |||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're returning true
here to indicate that the transaction was scheduled, but you pass true
to StartMultiLockedAhead()
to skip scheduling, is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function as a whole determines whether the script needs to schedule at all (not if no keys are present) and schedules. What I care about is whether it needs to schedule. if it already scheduled can be be looked up on the transaction itself
src/server/main_service.cc
Outdated
// If script runs on a single shard, we run it remotely to save hops | ||
if (!tx->IsScheduled() && tx->GetMultiMode() == Transaction::LOCK_AHEAD && | ||
tx->GetUniqueShardCnt() == 1) { | ||
DCHECK(*scheduled); // because tx multi mode is lock ahead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow the meaning of *scheduled == true
while !tx->IsScheduled()
- what does it mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means that we need to schedule but didn't yet, will update the names
src/server/main_service.cc
Outdated
if (*multi_mode != Transaction::NOT_DETERMINED) { | ||
StartMultiExec(cntx->db_index(), cntx->transaction, &exec_info, *multi_mode); | ||
StartMultiExec(cntx->db_index(), cntx->transaction, &exec_info, *multi_mode, delay_scheduling); | ||
scheduled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might depend on the value of delay_scheduling
, no?
I.e., shouldn't you do something like scheduled = StartMultiExec(...);
?
|
||
auto check_cb = [this](ShardId sid) { return !sharded_[sid].cmds.empty(); }; | ||
tx->PrepareSquashedMultiHop(base_cid_, check_cb); | ||
tx->ScheduleSingleHop(run_cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused - how can we do ScheduleSingleHop here if we determined that a single hop is not possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we scheduled above and this is no longer a single hop for the whole transaction, but rather just a multi call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth adding a comment why SSH here will perform multiple hops
src/server/transaction.cc
Outdated
|
||
if (multi_->role != SQUASHED_STUB) // stub transactions don't migrate between threads | ||
// stub transactions don't migrate between threads, so keep it's index cached |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// stub transactions don't migrate between threads, so keep it's index cached | |
// stub transactions don't migrate between threads, so keep its index cached |
86794b0
to
d160489
Compare
Still needs some polishment, I also need to check the replica side more in detail |
9760893
to
3228c04
Compare
args.async = false; | ||
CallFromScript(cntx, args); | ||
}); | ||
bool embedded = tx->GetMultiMode() != Transaction::NOT_DETERMINED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename embedded to was_undetermined
and reverse the condition here and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was_undetermined
doesn't explain why it should be so, whether it's embedded or not is what we care about and we check it by multi mode determinedness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- it's quite large change with lots of conditions. Do you think it is possible to split it into smaller ones?
- I do not see new tests besides that small unit test. We know for sure that we have a memory leak during the replication. If this PR fixes it, can you add a (py)test reproducing the slave leak and showing it's been fixed here?
I can split the core logic and then it's uses (eval, exec and squasher), but there is no way to test each part without the full changes. Besides they're all in separate places, so it's not the depth of changes, but the range, each part is independent
"memory leak"? Our current "fix" disables multi mode, so we just don't send the replication report. I will add a test |
|
||
// If all commands fit into a single batch, it can be run as a single hop, without separate hops for | ||
// locking and unlocking | ||
TEST_F(MultiTest, MultiSingleHop) { | ||
auto fb0 = pp_->at(0)->LaunchFiber([&] { | ||
for (unsigned i = 0; i < 100; i++) { | ||
Run({"multi"}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add another call to Run({"rpush", "a", "bar"}); inside the multi transaction here and check after execution size of a to check that both commands are executed
if (slot_id.has_value()) { | ||
unique_slot_checker_.Add(*slot_id); | ||
} | ||
MultiUpdateWithParent(parent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused param slot_id passed to constructor
Signed-off-by: Vladislav Oleshko <[email protected]>
fix: more fixes Signed-off-by: Vladislav Oleshko <[email protected]>
I wanted to simplify journaling so that stub transactions report directly to the main transaction, this way we don't need both |
consider rebasing |
Will do. This PR will become much easier because we don't need to use ScheduleSingleHop as it's no longer special |
Idea:
If we determine that squashed commands form only a single batch, we can use
ScheduleSingleHop()
for that batch, which:Without contended keys it gives no improvements 😞, but with a contended it's potentially up to two times faster 🙂