From cdee667170d9ee82262bc1f73f5f94ad11be219f Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 31 Jul 2024 19:57:32 +0300 Subject: [PATCH] alternator: destroy streamed json values gently Large json return values are streamed to avoid memory pressure and stalls, but are destroyed all at once. This in itself can cause stalls [1]. Destroy them gently to avoid the stalls. [1] ++[0#1/1 100%] addr=0x46880df total=514498 count=7004 avg=73: | seastar::backtrace at ./build/release/seastar.lto/./seastar/include/seastar/util/backtrace.hh:64 ++ - addr=0x4680b35: | seastar::backtrace_buffer::append_backtrace_oneline at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:839 | (inlined by) seastar::print_with_backtrace at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:858 ++ - addr=0x46800f7: | seastar::internal::cpu_stall_detector::generate_trace at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:1469 ++ - addr=0x4680178: | seastar::internal::cpu_stall_detector::maybe_report at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:1206 | (inlined by) seastar::internal::cpu_stall_detector::on_signal at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:1226 ++ - addr=0x3dbaf: ?? ??:0 ++[1#1/812 13%] addr=0x217b774 total=69336 count=990 avg=70: | rapidjson::GenericValue, rjson::internal::throwing_allocator>::~GenericValue at /usr/include/rapidjson/document.h:721 | ++[2#1/3 85%] addr=0x217b7db total=58974 count=842 avg=70: | | rapidjson::GenericMember, rjson::internal::throwing_allocator>::~GenericMember at /usr/include/rapidjson/document.h:71 | | (inlined by) rapidjson::GenericValue, rjson::internal::throwing_allocator>::~GenericValue at /usr/include/rapidjson/document.h:733 | | ++[3#1/4 45%] addr=0x217b7db total=902102 count=12903 avg=70: | | | rapidjson::GenericMember, rjson::internal::throwing_allocator>::~GenericMember at /usr/include/rapidjson/document.h:71 | | | (inlined by) rapidjson::GenericValue, rjson::internal::throwing_allocator>::~GenericValue at /usr/include/rapidjson/document.h:733 | | -> continued at addr=0x217b7db above | | |+[3#2/4 40%] addr=0x217b8b3 total=794219 count=11363 avg=70: | | | rapidjson::GenericValue, rjson::internal::throwing_allocator>::~GenericValue at /usr/include/rapidjson/document.h:726 | | | ++[4#1/1 100%] addr=0x217b7db total=909571 count=13012 avg=70: | | | | rapidjson::GenericMember, rjson::internal::throwing_allocator>::~GenericMember at /usr/include/rapidjson/document.h:71 | | | | (inlined by) rapidjson::GenericValue, rjson::internal::throwing_allocator>::~GenericValue at /usr/include/rapidjson/document.h:733 | | | -> continued at addr=0x217b7db above | | |+[3#3/4 15%] addr=0x43d35a3 total=296768 count=4246 avg=70: | | | seastar::shared_ptr_count_for, rjson::internal::throwing_allocator> >::~shared_ptr_count_for at ././seastar/include/seastar/core/shared_ptr.hh:492 | | | (inlined by) seastar::shared_ptr_count_for, rjson::internal::throwing_allocator> >::~shared_ptr_count_for at ././seastar/include/seastar/core/shared_ptr.hh:492 | | | ++[4#1/2 98%] addr=0x43e7d06 total=289680 count=4144 avg=70: | | | | seastar::shared_ptr, rjson::internal::throwing_allocator> >::~shared_ptr at ././seastar/include/seastar/core/shared_ptr.hh:570 | | | | (inlined by) alternator::make_streamed(rapidjson::GenericValue, rjson::internal::throwing_allocator>&&)::$_0::operator() at ./alternator/executor.cc:127 | | | ++ - addr=0x184e0a6: | | | | std::__n4861::coroutine_handle::promise_type>::resume at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/coroutine:240 | | | | (inlined by) seastar::internal::coroutine_traits_base::promise_type::run_and_dispose at ./build/release/seastar.lto/./seastar/include/seastar/core/coroutine.hh:125 | | | | (inlined by) seastar::reactor::run_tasks at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:2651 | | | | (inlined by) seastar::reactor::run_some_tasks at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:3114 | | | | ++[5#1/1 100%] addr=0x2503b87 total=310677 count=4417 avg=70: | | | | | seastar::reactor::do_run at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:3283 | | | | ++[6#1/2 78%] addr=0x46a2898 total=400571 count=5450 avg=73: | | | | | seastar::smp::configure(seastar::smp_options const&, seastar::reactor_options const&)::$_0::operator() at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:4501 | | | | | (inlined by) std::__invoke_impl at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/invoke.h:61 | | | | | (inlined by) std::__invoke_r at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/invoke.h:111 | | | | | (inlined by) std::_Function_handler::_M_invoke at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/std_function.h:290 | | | | ++ - addr=0x4673fda: | | | | | std::function::operator() at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/std_function.h:591 | | | | | (inlined by) seastar::posix_thread::start_routine at ./build/release/seastar.lto/./seastar/src/core/posix.cc:90 | | | | ++ - addr=0x8c946: ?? ??:0 | | | | ++ - addr=0x11296f: ?? ??:0 | | | | ++[6#2/2 22%] addr=0x2502c1e total=113613 count=1549 avg=73: | | | | | seastar::reactor::run at ./build/release/seastar.lto/./seastar/src/core/reactor.cc:3166 | | | | ++ - addr=0x22068e0: | | | | | seastar::app_template::run_deprecated at ./build/release/seastar.lto/./seastar/src/core/app-template.cc:276 | | | | ++ - addr=0x220630b: | | | | | seastar::app_template::run at ./build/release/seastar.lto/./seastar/src/core/app-template.cc:167 | | | | ++ - addr=0x22334bc: | | | | | scylla_main at ./main.cc:672 | | | | ++ - addr=0x20411cc: | | | | | std::function::operator() at /usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/std_function.h:591 | | | | | (inlined by) main at ./main.cc:2072 | | | | ++ - addr=0x27b89: ?? ??:0 | | | | ++ - addr=0x27c4a: ?? ??:0 | | | | ++ - addr=0x28c8fb4: | | | | | _start at ??:? Closes scylladb/scylladb#19968 --- alternator/executor.cc | 1 + utils/rjson.cc | 32 ++++++++++++++++++++++++++++++++ utils/rjson.hh | 6 ++++++ 3 files changed, 39 insertions(+) diff --git a/alternator/executor.cc b/alternator/executor.cc index 102b027dc04d..1dd6b7338fa6 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -116,6 +116,7 @@ json::json_return_type make_streamed(rjson::value&& value) { elogger.error("Exception during streaming HTTP response: {}", ex); } co_await los.close(); + co_await rjson::destroy_gently(std::move(*lrs)); if (ex) { co_await coroutine::return_exception_ptr(std::move(ex)); } diff --git a/utils/rjson.cc b/utils/rjson.cc index 41a11ea431fa..b9036af1fe65 100644 --- a/utils/rjson.cc +++ b/utils/rjson.cc @@ -556,4 +556,36 @@ sstring quote_json_string(const sstring& value) { return oss.str(); } +static future<> destroy_gently_nonleaf(rjson::value&& value_in) { + // We want the caller to move the value into this function, so 'value_in' is an rvalue reference. + // We want to hold the value while it's being destroyed, so we move it into the coroutine frame + // as the local 'value'. + auto value = std::move(value_in); + + if (value.IsObject()) { + for (auto it = value.MemberBegin(); it != value.MemberEnd();) { + co_await destroy_gently(std::move(it->value)); + it = value.EraseMember(it); + } + } else if (value.IsArray()) { + for (auto i = value.Size(); i > 0; --i) { + auto index = i - 1; + co_await destroy_gently(std::move(value[index])); + value.Erase(value.Begin() + index); + } + } +} + +future<> destroy_gently(rjson::value&& value) { + // Most nodes will be leaves, so we use a non-coroutine destroy_gently() for them. The + // few non-leaves will be handled by the coroutine destroy_gently_nonleaf(). We could have + // coded the whole thing as a non-coroutine, but that's more difficult and not worth the + // marginal improvement. + if (rjson::is_leaf(value)) { + return make_ready_future<>(); + } else { + return destroy_gently_nonleaf(std::move(value)); + } +} + } // end namespace rjson diff --git a/utils/rjson.hh b/utils/rjson.hh index accdd38a9b52..2b5a6f0a6ead 100644 --- a/utils/rjson.hh +++ b/utils/rjson.hh @@ -416,6 +416,12 @@ public: } }; +inline bool is_leaf(const rjson::value& value) { + return !value.IsObject() && !value.IsArray(); +} + +future<> destroy_gently(rjson::value&& value); + } // end namespace rjson template <> struct fmt::formatter {