mirror of
https://github.com/scylladb/scylladb.git
synced 2026-04-26 11:30:36 +00:00
Merge 'Don't throw exceptions on the replica side when handling single partition reads and writes' from Piotr Dulikowski
This PR gets rid of exception throws/rethrows on the replica side for writes and single-partition reads. This goal is achieved without using `boost::outcome` but rather by replacing the parts of the code which throw with appropriate seastar idioms and by introducing two helper functions:
1.`try_catch` allows to inspect the type and value behind an `std::exception_ptr`. When libstdc++ is used, this function does not need to throw the exception and avoids the very costly unwind process. This based on the "How to catch an exception_ptr without even try-ing" proposal mentioned in https://github.com/scylladb/scylla/issues/10260.
This function allows to replace the current `try..catch` chains which inspect the exception type and account it in the metrics.
Example:
```c++
// Before
try {
std::rethrow_exception(eptr);
} catch (std::runtime_exception& ex) {
// 1
} catch (...) {
// 2
}
// After
if (auto* ex = try_catch<std::runtime_exception>(eptr)) {
// 1
} else {
// 2
}
```
2. `make_nested_exception_ptr` which is meant to be a replacement for `std::throw_with_nested`. Unlike the original function, it does not require an exception being currently thrown and does not throw itself - instead, it takes the nested exception as an `std::exception_ptr` and produces another `std::exception_ptr` itself.
Apart from the above, seastar idioms such as `make_exception_future`, `co_await as_future`, `co_return coroutine::exception()` are used to propagate exceptions without throwing. This brings the number of exception throws to zero for single partition reads and writes (tested with scylla-bench, --mode=read and --mode=write).
Results from `perf_simple_query`:
```
Before (719724e4df):
Writes:
Normal:
127841.40 tps ( 56.2 allocs/op, 13.2 tasks/op, 50042 insns/op, 0 errors)
Timeouts:
94770.81 tps ( 53.1 allocs/op, 5.1 tasks/op, 78678 insns/op, 1000000 errors)
Reads:
Normal:
138902.31 tps ( 65.1 allocs/op, 12.1 tasks/op, 43106 insns/op, 0 errors)
Timeouts:
62447.01 tps ( 49.7 allocs/op, 12.1 tasks/op, 135984 insns/op, 936846 errors)
After (d8ac4c02bfb7786dc9ed30d2db3b99df09bf448f):
Writes:
Normal:
127359.12 tps ( 56.2 allocs/op, 13.2 tasks/op, 49782 insns/op, 0 errors)
Timeouts:
163068.38 tps ( 52.1 allocs/op, 5.1 tasks/op, 40615 insns/op, 1000000 errors)
Reads:
Normal:
151221.15 tps ( 65.1 allocs/op, 12.1 tasks/op, 43028 insns/op, 0 errors)
Timeouts:
192094.11 tps ( 41.2 allocs/op, 12.1 tasks/op, 33403 insns/op, 960604 errors)
```
Closes #10368
* github.com:scylladb/scylla:
database: avoid rethrows when handling exceptions from commitlog
database: convert throw_commitlog_add_error to use make_nested_exception_ptr
utils: add make_nested_exception_ptr
storage_proxy: don't rethrow when inspecting replica exceptions on write path
database: don't rethrow rate_limit_exception
storage_proxy: don't rethrow the exception in abstract_read_resolver::error
utils/exceptions.cc: don't rethrow in is_timeout_exception
utils/exceptions: add try_catch
utils: add abi/eh_ia64.hh
storage_proxy: don't rethrow exceptions from replicas when accounting read stats
message: get rid of throws in send_message{,_timeout,_abortable}
database/{query,query_mutations}: don't rethrow read semaphore exceptions
This commit is contained in:
11
test/boost/exceptions_fallback_test.cc
Normal file
11
test/boost/exceptions_fallback_test.cc
Normal file
@@ -0,0 +1,11 @@
|
||||
/*
|
||||
* Copyright (C) 2022-present ScyllaDB
|
||||
*/
|
||||
|
||||
/*
|
||||
* SPDX-License-Identifier: AGPL-3.0-or-later
|
||||
*/
|
||||
|
||||
#define NO_OPTIMIZED_EXCEPTION_HANDLING
|
||||
|
||||
#include "exceptions_test.inc.cc"
|
||||
30
test/boost/exceptions_optimized_test.cc
Normal file
30
test/boost/exceptions_optimized_test.cc
Normal file
@@ -0,0 +1,30 @@
|
||||
/*
|
||||
* Copyright (C) 2022-present ScyllaDB
|
||||
*/
|
||||
|
||||
/*
|
||||
* SPDX-License-Identifier: AGPL-3.0-or-later
|
||||
*/
|
||||
|
||||
#if defined(NO_OPTIMIZED_EXCEPTION_HANDLING)
|
||||
#undef NO_OPTIMIZED_EXCEPTION_HANDLING
|
||||
#endif
|
||||
|
||||
#include "utils/exceptions.hh"
|
||||
|
||||
#if defined(OPTIMIZED_EXCEPTION_HANDLING_AVAILABLE)
|
||||
|
||||
#include "exceptions_test.inc.cc"
|
||||
|
||||
#else
|
||||
|
||||
#include <boost/test/unit_test_log.hpp>
|
||||
#include <seastar/testing/test_case.hh>
|
||||
|
||||
SEASTAR_TEST_CASE(test_noop) {
|
||||
BOOST_TEST_MESSAGE("Optimized implementation of handling exceptions "
|
||||
"without throwing is not available. Skipping tests in this file.");
|
||||
return seastar::make_ready_future<>();
|
||||
}
|
||||
|
||||
#endif
|
||||
178
test/boost/exceptions_test.inc.cc
Normal file
178
test/boost/exceptions_test.inc.cc
Normal file
@@ -0,0 +1,178 @@
|
||||
/*
|
||||
* Copyright (C) 2022-present ScyllaDB
|
||||
*/
|
||||
|
||||
/*
|
||||
* SPDX-License-Identifier: AGPL-3.0-or-later
|
||||
*/
|
||||
|
||||
// Common definitions of test cases used in
|
||||
// handle_exception_optimized_test.cc
|
||||
// handle_exception_fallback_test.cc
|
||||
|
||||
#include <exception>
|
||||
#include <stdexcept>
|
||||
#include <type_traits>
|
||||
#include <cxxabi.h>
|
||||
#include <boost/test/unit_test_log.hpp>
|
||||
#include <boost/test/tools/old/interface.hpp>
|
||||
#include <boost/test/unit_test.hpp>
|
||||
|
||||
#include "seastarx.hh"
|
||||
#include <seastar/core/future.hh>
|
||||
#include <seastar/testing/test_case.hh>
|
||||
#include <seastar/util/log.hh>
|
||||
|
||||
#include "utils/exceptions.hh"
|
||||
|
||||
class base_exception : public std::exception {};
|
||||
class derived_exception : public base_exception {};
|
||||
|
||||
static void dummy_fn(void*) {
|
||||
//
|
||||
}
|
||||
|
||||
template<typename T>
|
||||
static std::exception_ptr maybe_wrap_eptr(T&& t) {
|
||||
if constexpr (std::is_same_v<T, std::exception_ptr>) {
|
||||
return std::move(t);
|
||||
} else {
|
||||
return std::make_exception_ptr(std::move(t));
|
||||
}
|
||||
}
|
||||
|
||||
static const std::type_info& eptr_typeid(std::exception_ptr eptr) {
|
||||
try {
|
||||
std::rethrow_exception(eptr);
|
||||
} catch (...) {
|
||||
return *abi::__cxa_current_exception_type();
|
||||
}
|
||||
}
|
||||
|
||||
template<typename Capture, typename Throw>
|
||||
static void check_catch(Throw&& ex) {
|
||||
auto eptr = maybe_wrap_eptr(std::move(ex));
|
||||
BOOST_TEST_MESSAGE("Checking if " << seastar::pretty_type_name(eptr_typeid(eptr))
|
||||
<< " is caught as " << seastar::pretty_type_name(typeid(Capture)));
|
||||
|
||||
auto typed_eptr = try_catch<Capture>(eptr);
|
||||
BOOST_CHECK_NE(typed_eptr, nullptr);
|
||||
|
||||
// Verify that it's the same as what the usual throwing gives
|
||||
// TODO: Does this check make sense? Does the standard guarantee
|
||||
// that this will give the same pointer?
|
||||
try {
|
||||
std::rethrow_exception(eptr);
|
||||
} catch (Capture& t) {
|
||||
BOOST_CHECK_EQUAL(typed_eptr, &t);
|
||||
} catch (...) {
|
||||
// Can happen if the first check fails, just skip
|
||||
assert(typed_eptr == nullptr);
|
||||
}
|
||||
}
|
||||
|
||||
template<typename Capture, typename Throw>
|
||||
static void check_no_catch(Throw&& ex) {
|
||||
auto eptr = maybe_wrap_eptr(std::move(ex));
|
||||
BOOST_TEST_MESSAGE("Checking if " << seastar::pretty_type_name(eptr_typeid(eptr))
|
||||
<< " is NOT caught as " << seastar::pretty_type_name(typeid(Capture)));
|
||||
|
||||
auto typed_eptr = try_catch<Capture>(eptr);
|
||||
BOOST_CHECK_EQUAL(typed_eptr, nullptr);
|
||||
}
|
||||
|
||||
template<typename A, typename B>
|
||||
static std::exception_ptr make_nested_eptr(A&& a, B&& b) {
|
||||
try {
|
||||
throw std::move(b);
|
||||
} catch (...) {
|
||||
try {
|
||||
std::throw_with_nested(std::move(a));
|
||||
} catch (...) {
|
||||
return std::current_exception();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
SEASTAR_TEST_CASE(test_try_catch) {
|
||||
// Some standard examples, throwing exceptions derived from std::exception
|
||||
// and catching them through their base types
|
||||
|
||||
check_catch<derived_exception>(derived_exception());
|
||||
check_catch<base_exception>(derived_exception());
|
||||
check_catch<std::exception>(derived_exception());
|
||||
check_no_catch<std::runtime_error>(derived_exception());
|
||||
|
||||
check_no_catch<derived_exception>(base_exception());
|
||||
check_catch<base_exception>(base_exception());
|
||||
check_catch<std::exception>(base_exception());
|
||||
check_no_catch<std::runtime_error>(base_exception());
|
||||
|
||||
// Catching nested exceptions
|
||||
check_catch<base_exception>(make_nested_eptr(base_exception(), derived_exception()));
|
||||
check_catch<std::nested_exception>(make_nested_eptr(base_exception(), derived_exception()));
|
||||
check_no_catch<derived_exception>(make_nested_eptr(base_exception(), derived_exception()));
|
||||
|
||||
// Check that everything works if we throw some crazy stuff
|
||||
check_catch<int>(int(1));
|
||||
check_no_catch<std::exception>(int(1));
|
||||
|
||||
check_no_catch<int>(nullptr);
|
||||
check_no_catch<std::exception>(nullptr);
|
||||
|
||||
// Catching pointers is not supported, but nothing should break if they are
|
||||
// being thrown
|
||||
derived_exception exc;
|
||||
check_no_catch<int>(&exc);
|
||||
check_no_catch<std::exception>(&exc);
|
||||
|
||||
check_no_catch<int>(&dummy_fn);
|
||||
check_no_catch<std::exception>(&dummy_fn);
|
||||
|
||||
check_no_catch<int>(&std::exception::what);
|
||||
check_no_catch<std::exception>(&std::exception::what);
|
||||
|
||||
return make_ready_future<>();
|
||||
}
|
||||
|
||||
SEASTAR_TEST_CASE(test_make_nested_exception_ptr) {
|
||||
auto inner = std::make_exception_ptr(std::runtime_error("inner"));
|
||||
auto outer = make_nested_exception_ptr(std::runtime_error("outer"), inner);
|
||||
|
||||
try {
|
||||
std::rethrow_exception(outer);
|
||||
} catch (const std::runtime_error& ex) {
|
||||
BOOST_REQUIRE_EQUAL(std::string(ex.what()), "outer");
|
||||
auto* nested = dynamic_cast<const std::nested_exception*>(&ex);
|
||||
BOOST_REQUIRE_NE(nested, nullptr);
|
||||
BOOST_REQUIRE_EQUAL(nested->nested_ptr(), inner);
|
||||
}
|
||||
|
||||
try {
|
||||
std::rethrow_exception(outer);
|
||||
} catch (const std::nested_exception& ex) {
|
||||
BOOST_REQUIRE_EQUAL(ex.nested_ptr(), inner);
|
||||
}
|
||||
|
||||
// Not a class
|
||||
BOOST_REQUIRE_THROW(std::rethrow_exception(make_nested_exception_ptr(int(123), inner)), int);
|
||||
|
||||
// Final, so cannot add the std::nested_exception mixin to it
|
||||
struct ultimate_exception final : public std::exception {};
|
||||
BOOST_REQUIRE_THROW(std::rethrow_exception(make_nested_exception_ptr(ultimate_exception(), inner)), ultimate_exception);
|
||||
|
||||
// Already derived from nested exception, so cannot put more to it
|
||||
struct already_nested_exception : public std::exception, public std::nested_exception {};
|
||||
auto inner2 = std::make_exception_ptr(std::runtime_error("inner2"));
|
||||
try {
|
||||
std::rethrow_exception(inner2);
|
||||
} catch (const std::runtime_error&) {
|
||||
try {
|
||||
std::rethrow_exception(make_nested_exception_ptr(already_nested_exception(), inner));
|
||||
} catch (const already_nested_exception& ex) {
|
||||
BOOST_REQUIRE_EQUAL(ex.nested_ptr(), inner2);
|
||||
}
|
||||
}
|
||||
|
||||
return make_ready_future<>();
|
||||
}
|
||||
Reference in New Issue
Block a user