diff --git a/BUILD b/BUILD index 375f6146d0a..dae2cea68c8 100644 --- a/BUILD +++ b/BUILD @@ -1918,8 +1918,10 @@ grpc_cc_library( hdrs = [ "src/core/lib/resource_quota/arena.h", ], + external_deps = ["absl/utility"], tags = ["grpc-autodeps"], deps = [ + "construct_destruct", "context", "event_engine_memory_allocator", "gpr_base", diff --git a/CMakeLists.txt b/CMakeLists.txt index 7738f383341..d7d7c366338 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -788,7 +788,6 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_c alts_tsi_handshaker_test) add_dependencies(buildtests_c alts_tsi_utils_test) add_dependencies(buildtests_c alts_zero_copy_grpc_protector_test) - add_dependencies(buildtests_c arena_test) add_dependencies(buildtests_c auth_context_test) add_dependencies(buildtests_c b64_test) add_dependencies(buildtests_c bad_server_response_test) @@ -944,6 +943,7 @@ if(gRPC_BUILD_TESTS) endif() add_dependencies(buildtests_cxx alts_util_test) add_dependencies(buildtests_cxx arena_promise_test) + add_dependencies(buildtests_cxx arena_test) add_dependencies(buildtests_cxx async_end2end_test) add_dependencies(buildtests_cxx auth_property_iterator_test) add_dependencies(buildtests_cxx authorization_matchers_test) @@ -4633,33 +4633,6 @@ target_link_libraries(alts_zero_copy_grpc_protector_test ) -endif() -if(gRPC_BUILD_TESTS) - -add_executable(arena_test - test/core/gpr/arena_test.cc -) - -target_include_directories(arena_test - PRIVATE - ${CMAKE_CURRENT_SOURCE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR}/include - ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} - ${_gRPC_RE2_INCLUDE_DIR} - ${_gRPC_SSL_INCLUDE_DIR} - ${_gRPC_UPB_GENERATED_DIR} - ${_gRPC_UPB_GRPC_GENERATED_DIR} - ${_gRPC_UPB_INCLUDE_DIR} - ${_gRPC_XXHASH_INCLUDE_DIR} - ${_gRPC_ZLIB_INCLUDE_DIR} -) - -target_link_libraries(arena_test - ${_gRPC_ALLTARGETS_LIBRARIES} - grpc_test_util -) - - endif() if(gRPC_BUILD_TESTS) @@ -7692,6 +7665,41 @@ target_link_libraries(arena_promise_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(arena_test + test/core/resource_quota/arena_test.cc + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +target_include_directories(arena_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(arena_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util +) + + endif() if(gRPC_BUILD_TESTS) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 5d119f4dd9a..4424acd06c3 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -3320,15 +3320,6 @@ targets: - test/core/tsi/alts/zero_copy_frame_protector/alts_zero_copy_grpc_protector_test.cc deps: - grpc_test_util -- name: arena_test - build: test - language: c - headers: [] - src: - - test/core/gpr/arena_test.cc - deps: - - grpc_test_util - uses_polling: false - name: auth_context_test build: test language: c @@ -4630,6 +4621,16 @@ targets: - gpr - upb uses_polling: false +- name: arena_test + gtest: true + build: test + language: c++ + headers: [] + src: + - test/core/resource_quota/arena_test.cc + deps: + - grpc_test_util + uses_polling: false - name: async_end2end_test gtest: true build: test diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 5cc75bead76..754aac38322 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -25,7 +25,6 @@ #include #include #include -#include #include #include "absl/status/statusor.h" diff --git a/src/core/lib/resource_quota/arena.cc b/src/core/lib/resource_quota/arena.cc index 37caf2453ba..a8e973e697a 100644 --- a/src/core/lib/resource_quota/arena.cc +++ b/src/core/lib/resource_quota/arena.cc @@ -20,8 +20,11 @@ #include "src/core/lib/resource_quota/arena.h" +#include #include +#include "absl/utility/utility.h" + #include #include "src/core/lib/gpr/alloc.h" @@ -49,7 +52,7 @@ Arena::~Arena() { Zone* z = last_zone_; while (z) { Zone* prev_z = z->prev; - z->~Zone(); + Destruct(z); gpr_free_aligned(z); z = prev_z; } @@ -71,6 +74,16 @@ std::pair Arena::CreateWithAlloc( } size_t Arena::Destroy() { + ManagedNewObject* p; + // Outer loop: clear the managed new object list. + // We do this repeatedly in case a destructor ends up allocating something. + while ((p = managed_new_head_.exchange(nullptr, std::memory_order_relaxed)) != + nullptr) { + // Inner loop: destruct a batch of objects. + while (p != nullptr) { + Destruct(absl::exchange(p, p->next)); + } + } size_t size = total_used_.load(std::memory_order_relaxed); memory_allocator_->Release(total_allocated_.load(std::memory_order_relaxed)); this->~Arena(); @@ -98,4 +111,11 @@ void* Arena::AllocZone(size_t size) { return reinterpret_cast(z) + zone_base_size; } +void Arena::ManagedNewObject::Link(std::atomic* head) { + next = head->load(std::memory_order_relaxed); + while (!head->compare_exchange_weak(next, this, std::memory_order_acq_rel, + std::memory_order_relaxed)) { + } +} + } // namespace grpc_core diff --git a/src/core/lib/resource_quota/arena.h b/src/core/lib/resource_quota/arena.h index a7aec0abcf4..39038a6b80b 100644 --- a/src/core/lib/resource_quota/arena.h +++ b/src/core/lib/resource_quota/arena.h @@ -31,12 +31,12 @@ #include #include -#include #include #include #include "src/core/lib/gpr/alloc.h" +#include "src/core/lib/gprpp/construct_destruct.h" #include "src/core/lib/promise/context.h" #include "src/core/lib/resource_quota/memory_quota.h" @@ -76,15 +76,36 @@ class Arena { template T* New(Args&&... args) { T* t = static_cast(Alloc(sizeof(T))); - new (t) T(std::forward(args)...); + Construct(t, std::forward(args)...); return t; } + // Like New, but has the arena call p->~T() at arena destruction time. + template + T* ManagedNew(Args&&... args) { + auto* p = New>(std::forward(args)...); + p->Link(&managed_new_head_); + return &p->t; + } + private: struct Zone { Zone* prev; }; + struct ManagedNewObject { + ManagedNewObject* next = nullptr; + void Link(std::atomic* head); + virtual ~ManagedNewObject() = default; + }; + + template + struct ManagedNewImpl : public ManagedNewObject { + T t; + template + explicit ManagedNewImpl(Args&&... args) : t(std::forward(args)...) {} + }; + // Initialize an arena. // Parameters: // initial_size: The initial size of the whole arena in bytes. These bytes @@ -118,6 +139,7 @@ class Arena { // and (2) the allocated memory. The arena itself maintains a pointer to the // last zone; the zone list is reverse-walked during arena destruction only. std::atomic last_zone_{nullptr}; + std::atomic managed_new_head_{nullptr}; // The backing memory quota MemoryAllocator* const memory_allocator_; }; diff --git a/test/core/gpr/BUILD b/test/core/gpr/BUILD index 2963d2b0b69..902be940706 100644 --- a/test/core/gpr/BUILD +++ b/test/core/gpr/BUILD @@ -30,18 +30,6 @@ grpc_cc_test( ], ) -grpc_cc_test( - name = "arena_test", - srcs = ["arena_test.cc"], - language = "C++", - uses_event_engine = False, - uses_polling = False, - deps = [ - "//:gpr", - "//test/core/util:grpc_test_util", - ], -) - grpc_cc_test( name = "cpu_test", srcs = ["cpu_test.cc"], diff --git a/test/core/gpr/arena_test.cc b/test/core/gpr/arena_test.cc deleted file mode 100644 index 6b9ece25088..00000000000 --- a/test/core/gpr/arena_test.cc +++ /dev/null @@ -1,136 +0,0 @@ -/* - * - * Copyright 2017 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#include "src/core/lib/resource_quota/arena.h" - -#include -#include - -#include "absl/strings/str_format.h" -#include "absl/strings/str_join.h" - -#include -#include -#include -#include - -#include "src/core/lib/gpr/string.h" -#include "src/core/lib/gpr/useful.h" -#include "src/core/lib/gprpp/thd.h" -#include "src/core/lib/resource_quota/resource_quota.h" -#include "test/core/util/test_config.h" - -using grpc_core::Arena; - -static auto* g_memory_allocator = new grpc_core::MemoryAllocator( - grpc_core::ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator( - "test")); - -static void test_noop(void) { Arena::Create(1, g_memory_allocator)->Destroy(); } - -static void test(const char* name, size_t init_size, const size_t* allocs, - size_t nallocs) { - std::vector parts; - parts.push_back( - absl::StrFormat("test '%s': %" PRIdPTR " <- {", name, init_size)); - for (size_t i = 0; i < nallocs; i++) { - parts.push_back(absl::StrFormat("%" PRIdPTR ",", allocs[i])); - } - parts.push_back("}"); - std::string s = absl::StrJoin(parts, ""); - gpr_log(GPR_INFO, "%s", s.c_str()); - - Arena* a = Arena::Create(init_size, g_memory_allocator); - void** ps = static_cast(gpr_zalloc(sizeof(*ps) * nallocs)); - for (size_t i = 0; i < nallocs; i++) { - ps[i] = a->Alloc(allocs[i]); - // ensure the returned address is aligned - GPR_ASSERT(((intptr_t)ps[i] & 0xf) == 0); - // ensure no duplicate results - for (size_t j = 0; j < i; j++) { - GPR_ASSERT(ps[i] != ps[j]); - } - // ensure writable - memset(ps[i], 1, allocs[i]); - } - a->Destroy(); - gpr_free(ps); -} - -#define TEST(name, init_size, ...) \ - static const size_t allocs_##name[] = {__VA_ARGS__}; \ - test(#name, init_size, allocs_##name, GPR_ARRAY_SIZE(allocs_##name)) - -#define CONCURRENT_TEST_THREADS 10 - -size_t concurrent_test_iterations() { - if (sizeof(void*) < 8) return 1000; - return 100000; -} - -typedef struct { - gpr_event ev_start; - Arena* arena; -} concurrent_test_args; - -static void concurrent_test_body(void* arg) { - concurrent_test_args* a = static_cast(arg); - gpr_event_wait(&a->ev_start, gpr_inf_future(GPR_CLOCK_REALTIME)); - for (size_t i = 0; i < concurrent_test_iterations(); i++) { - *static_cast(a->arena->Alloc(1)) = static_cast(i); - } -} - -static void concurrent_test(void) { - gpr_log(GPR_DEBUG, "concurrent_test"); - - concurrent_test_args args; - gpr_event_init(&args.ev_start); - args.arena = Arena::Create(1024, g_memory_allocator); - - grpc_core::Thread thds[CONCURRENT_TEST_THREADS]; - - for (int i = 0; i < CONCURRENT_TEST_THREADS; i++) { - thds[i] = - grpc_core::Thread("grpc_concurrent_test", concurrent_test_body, &args); - thds[i].Start(); - } - - gpr_event_set(&args.ev_start, reinterpret_cast(1)); - - for (auto& th : thds) { - th.Join(); - } - - args.arena->Destroy(); -} - -int main(int argc, char* argv[]) { - grpc::testing::TestEnvironment env(&argc, argv); - - test_noop(); - TEST(0_1, 0, 1); - TEST(1_1, 1, 1); - TEST(1_2, 1, 2); - TEST(1_3, 1, 3); - TEST(1_inc, 1, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11); - TEST(6_123, 6, 1, 2, 3); - concurrent_test(); - - return 0; -} diff --git a/test/core/resource_quota/BUILD b/test/core/resource_quota/BUILD index f0c5de22011..451ee3edb26 100644 --- a/test/core/resource_quota/BUILD +++ b/test/core/resource_quota/BUILD @@ -19,6 +19,21 @@ licenses(["notice"]) grpc_package(name = "test/core/resource_quota") +grpc_cc_test( + name = "arena_test", + srcs = ["arena_test.cc"], + external_deps = [ + "gtest", + ], + language = "C++", + uses_event_engine = False, + uses_polling = False, + deps = [ + "//:gpr", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_library( name = "call_checker", testonly = True, diff --git a/test/core/resource_quota/arena_test.cc b/test/core/resource_quota/arena_test.cc new file mode 100644 index 00000000000..b53406cb058 --- /dev/null +++ b/test/core/resource_quota/arena_test.cc @@ -0,0 +1,171 @@ +/* + * + * Copyright 2017 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "src/core/lib/resource_quota/arena.h" + +#include +#include + +#include +#include +#include +#include + +#include "absl/memory/memory.h" +#include "absl/strings/str_join.h" +#include "gtest/gtest.h" + +#include +#include +#include + +#include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/thd.h" +#include "src/core/lib/resource_quota/resource_quota.h" + +using grpc_core::Arena; + +static auto* g_memory_allocator = new grpc_core::MemoryAllocator( + grpc_core::ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator( + "test")); + +TEST(ArenaTest, NoOp) { Arena::Create(1, g_memory_allocator)->Destroy(); } + +TEST(ArenaTest, ManagedNew) { + Arena* arena = Arena::Create(1, g_memory_allocator); + for (int i = 0; i < 100; i++) { + arena->ManagedNew>(absl::make_unique(i)); + } + arena->Destroy(); +} + +struct AllocShape { + size_t initial_size; + std::vector allocs; +}; + +std::ostream& operator<<(std::ostream& out, const AllocShape& shape) { + out << "AllocShape{initial_size=" << shape.initial_size + << ", allocs=" << absl::StrJoin(shape.allocs, ",") << "}"; + return out; +} + +class AllocTest : public ::testing::TestWithParam {}; + +TEST_P(AllocTest, Works) { + Arena* a = Arena::Create(GetParam().initial_size, g_memory_allocator); + std::vector allocated; + for (auto alloc : GetParam().allocs) { + void* p = a->Alloc(alloc); + // ensure the returned address is aligned + EXPECT_EQ(((intptr_t)p & 0xf), 0); + // ensure no duplicate results + for (auto other_p : allocated) { + EXPECT_NE(p, other_p); + } + // ensure writable + memset(p, 1, alloc); + allocated.push_back(p); + } + a->Destroy(); +} + +INSTANTIATE_TEST_SUITE_P( + AllocTests, AllocTest, + ::testing::Values(AllocShape{0, {1}}, AllocShape{1, {1}}, + AllocShape{1, {2}}, AllocShape{1, {3}}, + AllocShape{1, {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}}, + AllocShape{6, {1, 2, 3}})); + +#define CONCURRENT_TEST_THREADS 10 + +size_t concurrent_test_iterations() { + if (sizeof(void*) < 8) return 1000; + return 100000; +} + +typedef struct { + gpr_event ev_start; + Arena* arena; +} concurrent_test_args; + +TEST(ArenaTest, ConcurrentAlloc) { + concurrent_test_args args; + gpr_event_init(&args.ev_start); + args.arena = Arena::Create(1024, g_memory_allocator); + + grpc_core::Thread thds[CONCURRENT_TEST_THREADS]; + + for (int i = 0; i < CONCURRENT_TEST_THREADS; i++) { + thds[i] = grpc_core::Thread( + "grpc_concurrent_test", + [](void* arg) { + concurrent_test_args* a = static_cast(arg); + gpr_event_wait(&a->ev_start, gpr_inf_future(GPR_CLOCK_REALTIME)); + for (size_t i = 0; i < concurrent_test_iterations(); i++) { + *static_cast(a->arena->Alloc(1)) = static_cast(i); + } + }, + &args); + thds[i].Start(); + } + + gpr_event_set(&args.ev_start, reinterpret_cast(1)); + + for (auto& th : thds) { + th.Join(); + } + + args.arena->Destroy(); +} + +TEST(ArenaTest, ConcurrentManagedNew) { + concurrent_test_args args; + gpr_event_init(&args.ev_start); + args.arena = Arena::Create(1024, g_memory_allocator); + + grpc_core::Thread thds[CONCURRENT_TEST_THREADS]; + + for (int i = 0; i < CONCURRENT_TEST_THREADS; i++) { + thds[i] = grpc_core::Thread( + "grpc_concurrent_test", + [](void* arg) { + concurrent_test_args* a = static_cast(arg); + gpr_event_wait(&a->ev_start, gpr_inf_future(GPR_CLOCK_REALTIME)); + for (size_t i = 0; i < concurrent_test_iterations(); i++) { + a->arena->ManagedNew>( + absl::make_unique(static_cast(i))); + } + }, + &args); + thds[i].Start(); + } + + gpr_event_set(&args.ev_start, reinterpret_cast(1)); + + for (auto& th : thds) { + th.Join(); + } + + args.arena->Destroy(); +} + +int main(int argc, char* argv[]) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index fa8647c8c78..35973842f95 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -313,30 +313,6 @@ ], "uses_polling": true }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": false, - "language": "c", - "name": "arena_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": false - }, { "args": [], "benchmark": false, @@ -2659,6 +2635,30 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "arena_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,