Report SS2PL lock acquisition failures to the caller - #122
Merged
Conversation
`TxExecutor::read()` and `TxExecutor::update()` in `cc/ss2pl/transaction.cc` set `status_ = aborted` when a lock cannot be taken, and then return `Status::OK` anyway. A caller that looks at the return value sees success, so `read()` leaves the caller's `TupleBody*` pointing at an unrelated `read_set_` entry (or at nothing, when the read set is empty) and the caller dereferences it. `tpcc_ss2pl.exe` dies from this deterministically. TPC-C Payment updates the Warehouse row of every transaction, which is the most contended row in the benchmark, so with two or more threads a write lock conflict happens within seconds. Measured on an Intel Xeon Platinum 8468 (48 cores), Release, g++ 11.4.0, default `CCBENCH_SS2PL_DLR=1`, `-extime=3`, three runs per thread count: | `-thread_num` | before | after | |---|---|---| | 1 | 3/3 exit 0 | 3/3 exit 0 | | 2 | 3/3 SIGSEGV | 3/3 exit 0 | | 4 | 3/3 SIGSEGV | 3/3 exit 0 | | 8 | 3/3 SIGSEGV | 3/3 exit 0 | | 48 | 3/3 SIGSEGV | 3/3 exit 0 | Both columns come from the same job on the same host, so the failure and the fix are measured against each other rather than against a remembered result. PR #121 added exactly this guard, but only inside `#if defined(DLR0)`, because that change had promised not to touch the DLR1 code at all. The defect is in DLR1 as well, and DLR1 is the default, so the guards become unconditional here. - `cc/ss2pl/transaction.cc`: the two `#if defined(DLR0)` / `#endif` pairs around the `status_ == aborted` guards in `read()` and `update()` are removed. Nothing inside them changes, and neither does any lock acquisition branch. `scan()`, `read_lock()` and `write_lock()` already returned `Status::ERROR_LOCK_FAILED` unconditionally, so this makes the five `Status`-returning operations agree with each other. - `include/bomb_pessimistic.hh`: `run_update_material_cost_master()` checked neither the return value nor `status_` before dereferencing `body`. This is the pre-existing problem that the #121 commit message listed as deliberately unaddressed; it has to be handled now, because the new return value stops `read()` from writing `*body` at all. It now follows the form that `docs/coding-conventions_ja.md` prescribes and that `get_material_cost()` fifteen lines below already uses. This header is included only by `cc/ss2pl/bomb_ss2pl.cc`, so no other protocol is affected. - `include/tpcc/tpcc_tx_delivery.hh`: in `update_order_line_and_get_ol_total()`, `if (status != Status::OK) ERR;` ran before the `status_ == aborted` check, so a reported lock failure would call `ERR` and `exit(1)`. The two lines are swapped, which matches `update_customer_balance()` in the same file and the `scan()` handling fifteen lines above. The only state whose behaviour changes is `status != Status::OK && status_ == aborted`, which used to end the process and is now an ordinary abort and retry. - `docs/coding-conventions_{ja,en}.md`: the description of `tx.read`'s result listed only `OK` and `WARN_NOT_FOUND`. Protocol-specific errors are now mentioned, and the rule is stated as "reject every non-OK value" rather than as a test against one constant. The delivery fix is not optional. Building only the `cc/ss2pl/transaction.cc` half of this commit and running `tpcc_ss2pl.exe -thread_num=48 -extime=3` gives exit code 1 with `ERR` from `tpcc_tx_delivery.hh` in all three runs: the SIGSEGV is replaced by a process abort rather than removed. With the caller adaptations, the same command exits 0. Callers audited by following the reference graph rather than by grepping. `cc/ss2pl/CMakeLists.txt` builds `ycsb`, `bomb` and `tpcc`; `cc/ss2pl/test/` is not added by any `add_subdirectory` and is not built. Those three entrypoints reach 34 lexical `read()` / `update()` call sites, 31 of them reachable (`include/bomb_pessimistic.hh` lines 904, 913 and 930 are in helpers that nothing calls). Every TPC-C site already tested `stat != Status::OK`, so they now decline safely. Every YCSB site tests `tx.status_` immediately after the call, so its retry behaviour is unchanged. Only the BoMB site above tested neither. No regression, same host and job, 48 threads, `-extime=5`, three runs each, median reported: | workload | before | after | |---|---|---| | YCSB, 1M records, zipf 0.9 | 682,581 tps / 0.8731 | 680,706 tps / 0.8743 | | YCSB, 1M records, uniform | 7,621,903 tps / 0.0031 | 7,618,854 tps / 0.0032 | | YCSB, 100 records, zipf 0.9 | 439,287 tps / 0.2290 | 440,740 tps / 0.2378 | | BoMB, 8 threads | 19,654 tps / 0.9887 | 20,175 tps / 0.9794 | | BoMB, 48 threads | 23,211 tps / 0.9955 | 22,394 tps / 0.9922 | DLR0 keeps the contract from #121. Its lock and timeout implementation is not touched: `cc/ss2pl/include/dlr0_timeout.hh`, `cc/ss2pl/util.cc`, `cc/ss2pl/CMakeLists.txt`, `cmake/Options.cmake` and `include/rwlock.hh` have no diff. What changes for DLR0 is that its callers now handle the `ERROR_LOCK_FAILED` it already returned. Measured with `CCBENCH_SS2PL_DLR=0`: the cycle-forming configuration (YCSB-A, 1,000,000 records, zipf skew 0.9, 48 threads) finishes in 6 s at the default timeout, the same configuration with `-ss2pl_dlr0_timeout_us=3600000000` is still running when a 120 s external watchdog kills it, and `tpcc_ss2pl.exe` exits 0 at 2, 8 and 48 threads. The reordered lines live in a header shared by every protocol that builds TPC-C, so `tpcc_<protocol>.exe -thread_num=48 -extime=3` was run before and after, one run each. All eight other protocols end the same way in both: `tictoc`, `cicada` and `mocc` exit 0; `silo` and `mvto` are still running when a 300 s watchdog kills them; `si` and `ermia` exit 1 through an `ERR` in their own `garbage_collection.cc`. `oze` aborted before and exited 0 after, but from `std::out_of_range` thrown by `std::map::at` inside its own `TxExecutor::write_validation()`, which this change does not touch; with one run each that difference is not attributable here. In other words, no other protocol was observed reaching the state whose handling changed. Release builds of every protocol with `-Wall -Wextra -Werror` pass for `CCBENCH_SS2PL_DLR` 0 and 1 with both g++ 11.4.0 and g++ 12.3.0. The `clang-format --dry-run --Werror` check from `.github/workflows/format.yml` passes with clang-format 14. Not verified: GCC 13. The CI image uses it; it was not available on the test machine, so the `-Werror` result under the CI compiler is unconfirmed. Deliberately not addressed here: - `include/ycsb.hh` discards the `read()` return value. A lock failure is still caught by the `status_` check that follows, but a `WARN_NOT_FOUND` would not be. YCSB creates every key it reads, so this is latent. - `include/bomb.hh`, `include/sbomb_deterministic.hh` and `include/dbomb_deterministic.hh` have the same unchecked read as the BoMB header fixed here. They belong to the other protocols and none of those protocols' `read()` currently reaches this state in the same way. - Other protocols have their own instances of setting `status_ = aborted` while returning `Status::OK` (cicada, ermia, mocc, oze, si, silo and tictoc, mostly in `scan()` and its callback). One branch, one purpose: they are reported, not changed. - `tpcc_ss2pl.exe` reports "insert order failed" heavily even single-threaded, on the parent commit as well as here, and the count moves with this change (15,046 before, 13,005 after, one thread, 3 s). The cause was not established, so no claim is made about it either way. This change was written with AI assistance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TxExecutor::read()andTxExecutor::update()incc/ss2pl/transaction.ccsetstatus_ = abortedwhen a lock cannot be taken, and then returnStatus::OKanyway. A caller that looks at the return value sees success, soread()leaves the caller'sTupleBody*pointing at an unrelatedread_set_entry (or at nothing, when the read set is empty) and the caller dereferences it.tpcc_ss2pl.exedies from this deterministically. TPC-C Payment updates the Warehouse row of every transaction, which is the most contended row in the benchmark, so with two or more threads a write lock conflict happens within seconds. Measured on an Intel Xeon Platinum 8468 (48 cores), Release, g++ 11.4.0, defaultCCBENCH_SS2PL_DLR=1,-extime=3, three runs per thread count:-thread_numBoth columns come from the same job on the same host, so the failure and the fix are measured against each other rather than against a remembered result.
PR #121 added exactly this guard, but only inside
#if defined(DLR0), because that change had promised not to touch the DLR1 code at all. The defect is in DLR1 as well, and DLR1 is the default, so the guards become unconditional here.cc/ss2pl/transaction.cc: the two#if defined(DLR0)/#endifpairs around thestatus_ == abortedguards inread()andupdate()are removed. Nothing inside them changes, and neither does any lock acquisition branch.scan(),read_lock()andwrite_lock()already returnedStatus::ERROR_LOCK_FAILEDunconditionally, so this makes the fiveStatus-returning operations agree with each other.include/bomb_pessimistic.hh:run_update_material_cost_master()checked neither the return value norstatus_before dereferencingbody. This is the pre-existing problem that the Implement the timeout that DLR0 documents #121 commit message listed as deliberately unaddressed; it has to be handled now, because the new return value stopsread()from writing*bodyat all. It now follows the form thatdocs/coding-conventions_ja.mdprescribes and thatget_material_cost()fifteen lines below already uses. This header is included only bycc/ss2pl/bomb_ss2pl.cc, so no other protocol is affected.include/tpcc/tpcc_tx_delivery.hh: inupdate_order_line_and_get_ol_total(),if (status != Status::OK) ERR;ran before thestatus_ == abortedcheck, so a reported lock failure would callERRandexit(1). The two lines are swapped, which matchesupdate_customer_balance()in the same file and thescan()handling fifteen lines above. The only state whose behaviour changes isstatus != Status::OK && status_ == aborted, which used to end the process and is now an ordinary abort and retry.docs/coding-conventions_{ja,en}.md: the description oftx.read's result listed onlyOKandWARN_NOT_FOUND. Protocol-specific errors are now mentioned, and the rule is stated as "reject every non-OK value" rather than as a test against one constant.The delivery fix is not optional. Building only the
cc/ss2pl/transaction.cchalf of this commit and runningtpcc_ss2pl.exe -thread_num=48 -extime=3gives exit code 1 withERRfromtpcc_tx_delivery.hhin all three runs: the SIGSEGV is replaced by a process abort rather than removed. With the caller adaptations, the same command exits 0.Callers audited by following the reference graph rather than by grepping.
cc/ss2pl/CMakeLists.txtbuildsycsb,bombandtpcc;cc/ss2pl/test/is not added by anyadd_subdirectoryand is not built. Those three entrypoints reach 34 lexicalread()/update()call sites, 31 of them reachable (include/bomb_pessimistic.hhlines 904, 913 and 930 are in helpers that nothing calls). Every TPC-C site already testedstat != Status::OK, so they now decline safely. Every YCSB site teststx.status_immediately after the call, so its retry behaviour is unchanged. Only the BoMB site above tested neither.No regression, same host and job, 48 threads,
-extime=5, three runs each, median reported:DLR0 keeps the contract from #121. Its lock and timeout implementation is not touched:
cc/ss2pl/include/dlr0_timeout.hh,cc/ss2pl/util.cc,cc/ss2pl/CMakeLists.txt,cmake/Options.cmakeandinclude/rwlock.hhhave no diff. What changes for DLR0 is that its callers now handle theERROR_LOCK_FAILEDit already returned. Measured withCCBENCH_SS2PL_DLR=0: the cycle-forming configuration (YCSB-A, 1,000,000 records, zipf skew 0.9, 48 threads) finishes in 6 s at the default timeout, the same configuration with-ss2pl_dlr0_timeout_us=3600000000is still running when a 120 s external watchdog kills it, andtpcc_ss2pl.exeexits 0 at 2, 8 and 48 threads.The reordered lines live in a header shared by every protocol that builds TPC-C, so
tpcc_<protocol>.exe -thread_num=48 -extime=3was run before and after, one run each. All eight other protocols end the same way in both:tictoc,cicadaandmoccexit 0;siloandmvtoare still running when a 300 s watchdog kills them;siandermiaexit 1 through anERRin their owngarbage_collection.cc.ozeaborted before and exited 0 after, but fromstd::out_of_rangethrown bystd::map::atinside its ownTxExecutor::write_validation(), which this change does not touch; with one run each that difference is not attributable here. In other words, no other protocol was observed reaching the state whose handling changed.Release builds of every protocol with
-Wall -Wextra -Werrorpass forCCBENCH_SS2PL_DLR0 and 1 with both g++ 11.4.0 and g++ 12.3.0. Theclang-format --dry-run --Werrorcheck from.github/workflows/format.ymlpasses with clang-format 14.Not verified: GCC 13. The CI image uses it; it was not available on the test machine, so the
-Werrorresult under the CI compiler is unconfirmed.Deliberately not addressed here:
include/ycsb.hhdiscards theread()return value. A lock failure is still caught by thestatus_check that follows, but aWARN_NOT_FOUNDwould not be. YCSB creates every key it reads, so this is latent.include/bomb.hh,include/sbomb_deterministic.hhandinclude/dbomb_deterministic.hhhave the same unchecked read as the BoMB header fixed here. They belong to the other protocols and none of those protocols'read()currently reaches this state in the same way.status_ = abortedwhile returningStatus::OK(cicada, ermia, mocc, oze, si, silo and tictoc, mostly inscan()and its callback). One branch, one purpose: they are reported, not changed.tpcc_ss2pl.exereports "insert order failed" heavily even single-threaded, on the parent commit as well as here, and the count moves with this change (15,046 before, 13,005 after, one thread, 3 s). The cause was not established, so no claim is made about it either way.This change was written with AI assistance.