feat(transaction): Idempotent callbacks (immediate runs) #2453

dranikpg · 2024-01-22T07:51:11Z

This PR generalizes the machanism of running transaction callbacks during scheduling, removing the need for specialized ScheduleUniqueShard/RunQuickie. Instead, transactions can be run now during ScheduleInShard - called "immediate" runs - if the transaction is concluding and either only a single shard is active or the operation can be safely repeated if scheduling failed (idempotent commands, like MGET).

Updates transaction stats to mirror the new changes more closely.

dranikpg · 2024-01-22T12:01:12Z

I want to rebase it on #2455 because currently the code is hard to deal with

dranikpg · 2024-02-20T06:57:22Z

I removed ScheduleSingleHop 😈

dranikpg · 2024-03-21T16:37:14Z

src/server/transaction.cc

+    // This is a contention point for all threads - avoid using it unless necessary.
+    // Single shard operations can assign txid later if the immediate run failed.
+    if (unique_shard_cnt_ > 1)
+      txid_ = op_seq.fetch_add(1, memory_order_relaxed);
+


Assigning txid unconditionally was costing about ~5% on 8 threads (and will do more on more threads)

The results are (8 threads):

basic, pipeline 20 new: 1.77M old: 1.76M mget 3 keys, pipeline 20 new: 950k old: 670k, txq warnings ~100 len

so its +- the same for single key and +40% for mget 😎

src/server/transaction.cc

romange · 2024-03-21T17:54:29Z

src/server/transaction.cc

+      shard->stats().tx_immediate_total++;
+
+      RunCallback(shard);
+      if (coordinator_state_ & COORD_CONCLUDING)  // could've been cleared by AVOID_CONCLUDING


is it ok that you access coordinator_state_ from the shard thread?

Yes, just reading is fine. I write only during "avoid concluding" optimization that is issued only for single shard operations (checking)

I did not fully understand your comment but please add a comment to the code because it's an exception to the rule about coordinator_state_ access.

src/server/transaction.cc

romange · 2024-03-21T17:58:57Z

please add a extensive commit description for the change. It's pretty fundamental. You change a two year old tx design, so worth at good explanation. You can squash commits too

Signed-off-by: Vladislav Oleshko <[email protected]> chore: Replace Schedule and ScheduleSingleHop with no-op chore: fix blocking concluding chore: Allow inlined runs and optimize txid chore: Move more stuff into RunCallback Signed-off-by: Vladislav Oleshko <[email protected]> chore: Enable metrics for new tx Signed-off-by: Vladislav Oleshko <[email protected]>

src/server/server_family.cc

src/server/string_family.cc

romange

Great work!

romange · 2024-03-24T16:12:40Z

src/server/transaction.cc

-    shard_set->RunBriefInParallel(std::move(cb), is_active);
+
+    run_barrier_.Start(unique_shard_cnt_);
+    if (CanRunInlined()) {


why not?

if (CanRunInlined()) { CHECK(ScheduleInShard(EngineShard::tlocal(), can_run_immediately)); } else { auto cb = .. run_barrier_.Start(unique_shard_cnt_); IterateActiveShards([cb](const auto& sd, ShardId i) { shard_set->Add(i, cb); }); run_barrier_.Wait(); }

True, good idea... I just thought you don't like such checks 😄

It fails our DCHECK logic for the barrier being active, I changed it a little to make it fit

src/server/transaction.cc

romange · 2024-03-24T16:23:01Z

src/server/transaction.cc

+      shard->stats().tx_immediate_total++;
+
+      RunCallback(shard);
+      if (coordinator_state_ & COORD_CONCLUDING)  // could've been cleared by AVOID_CONCLUDING


I did not fully understand your comment but please add a comment to the code because it's an exception to the rule about coordinator_state_ access.

romange

lgtm. Please wait until we release 1.16

Signed-off-by: Vladislav <[email protected]>

dranikpg force-pushed the tx-opt-inline-ooo branch 3 times, most recently from 327d63d to 4e9338a Compare February 20, 2024 06:37

dranikpg force-pushed the tx-opt-inline-ooo branch 6 times, most recently from d02a6d4 to 307a9bb Compare March 21, 2024 16:27

dranikpg commented Mar 21, 2024

View reviewed changes

dranikpg force-pushed the tx-opt-inline-ooo branch from f574338 to 3a917b0 Compare March 21, 2024 17:23

dranikpg changed the title ~~WIP feat(transaction): Idempotent callbacks (immediate runs)~~ feat(transaction): Idempotent callbacks (immediate runs) Mar 21, 2024

dranikpg requested a review from romange March 21, 2024 17:42