diff --git a/benchmarks/bench_parser_comparison.cpp b/benchmarks/bench_parser_comparison.cpp index e371df7..01e7418 100644 --- a/benchmarks/bench_parser_comparison.cpp +++ b/benchmarks/bench_parser_comparison.cpp @@ -76,13 +76,6 @@ private: Precondition current_precondition; Operation current_operation; - // Helper to store string in arena and return string_view - std::string_view store_string(const char *str, size_t length) { - char *stored = arena.allocate(length); - std::memcpy(stored, str, length); - return std::string_view(stored, length); - } - public: explicit CommitRequestArenaHandler() : preconditions(ArenaStlAllocator(&arena)), @@ -109,7 +102,7 @@ public: bool RawNumber(const char *, rapidjson::SizeType, bool) { abort(); } bool String(const char *str, rapidjson::SizeType length, bool) { - std::string_view value = store_string(str, length); + std::string_view value = arena.copy_string({str, length}); if (state == State::Root) { if (current_key == "request_id") { diff --git a/src/arena.hpp b/src/arena.hpp index efe85dc..61f7c33 100644 --- a/src/arena.hpp +++ b/src/arena.hpp @@ -405,7 +405,6 @@ public: * ## Note: * This method only allocates memory - it does not construct objects. * Use placement new or other initialization methods as needed. - * TODO should this return a std::span ? */ template T *allocate(uint32_t size) { static_assert( @@ -427,6 +426,72 @@ public: return static_cast(ptr); } + /** + * @brief Allocate an array of type T and return it as a std::span. + * + * This method provides bounds-safe allocation by returning a std::span + * that knows its size, improving safety over raw pointer allocation. + * + * @tparam T The type to allocate (must be trivially destructible) + * @param count The number of elements to allocate + * @return std::span A span covering the allocated array + * + * ## Safety: + * The returned span is valid for the lifetime of the arena and until + * the next reset() call. The span provides bounds checking in debug + * builds and clear size information. + * + * ## Usage: + * ```cpp + * auto buffer = arena.allocate_span(1024); + * auto strings = arena.allocate_span(10); + * ``` + * + * ## Note: + * Returns an empty span (nullptr, 0) if count is 0. + * This method only allocates memory - it does not construct objects. + */ + template std::span allocate_span(uint32_t count) { + if (count == 0) { + return std::span{}; + } + return std::span{allocate(count), count}; + } + + /** + * @brief Copy a string into arena memory and return a string_view. + * + * This method provides a safe way to copy string data into arena-allocated + * memory, ensuring the data remains valid for the arena's lifetime. + * + * @param str The string to copy into arena memory + * @return std::string_view pointing to the arena-allocated copy + * + * ## Safety: + * The returned string_view is valid for the lifetime of the arena and until + * the next reset() call. The string data is guaranteed to be null-terminated + * only if the input string was null-terminated. + * + * ## Usage: + * ```cpp + * Arena arena; + * std::string_view copy = arena.copy_string("Hello World"); + * std::string_view copy2 = arena.copy_string(some_string_view); + * ``` + * + * ## Note: + * Returns an empty string_view if the input string is empty. + * This method allocates exactly str.size() bytes (no null terminator added). + */ + std::string_view copy_string(std::string_view str) { + if (str.empty()) { + return std::string_view{}; + } + char *copied = allocate(str.size()); + std::memcpy(copied, str.data(), str.size()); + return std::string_view(copied, str.size()); + } + /** * @brief Reset the allocator to reuse the first block, freeing all others. * diff --git a/src/commit_request.hpp b/src/commit_request.hpp index 35a93c0..8cc51b9 100644 --- a/src/commit_request.hpp +++ b/src/commit_request.hpp @@ -244,14 +244,7 @@ public: * @return String view pointing to arena-allocated memory */ std::string_view copy_to_arena(std::string_view str) { - if (str.empty()) { - return {}; - } - - char *arena_str = arena_.allocate(str.size()); - std::memcpy(arena_str, str.data(), str.size()); - - return std::string_view(arena_str, str.size()); + return arena_.copy_string(str); } /** diff --git a/src/connection.hpp b/src/connection.hpp index 669d5aa..56b5bf0 100644 --- a/src/connection.hpp +++ b/src/connection.hpp @@ -44,7 +44,7 @@ struct MessageSender { * * Example usage: * ```cpp - * auto response_parts = std::span{arena.allocate(2), 2}; + * auto response_parts = arena.allocate_span(2); * response_parts[0] = "HTTP/1.1 200 OK\r\n\r\n"; * response_parts[1] = "Hello World"; * conn.append_message(response_parts, std::move(arena)); @@ -121,7 +121,7 @@ struct Connection : MessageSender { * Example usage: * ```cpp * Arena arena; - * auto* parts = arena.allocate(2); + * auto parts = arena.allocate_span(2); * parts[0] = build_header(arena); * parts[1] = build_body(arena); * conn.append_message({parts, 2}, std::move(arena)); diff --git a/src/http_handler.cpp b/src/http_handler.cpp index 5db0e51..1e63cdd 100644 --- a/src/http_handler.cpp +++ b/src/http_handler.cpp @@ -389,9 +389,8 @@ void HttpHandler::handle_get_metrics(Connection &conn, "\r\n", "Connection: keep-alive\r\n", "\r\n"); } - auto result = std::span{ - state.arena.allocate(metrics_span.size() + 1), - metrics_span.size() + 1}; + auto result = + state.arena.allocate_span(metrics_span.size() + 1); auto out = result.begin(); *out++ = headers; for (auto sv : metrics_span) { @@ -443,7 +442,7 @@ void HttpHandler::send_response(MessageSender &conn, int status_code, const char *connection_header = close_connection ? "close" : "keep-alive"; - auto response = std::span{response_arena.allocate(1), 1}; + auto response = response_arena.allocate_span(1); response[0] = format(response_arena, @@ -509,7 +508,7 @@ HttpHandler::format_response(int status_code, std::string_view content_type, const char *connection_header = close_connection ? "close" : "keep-alive"; - auto response = std::span{response_arena.allocate(1), 1}; + auto response = response_arena.allocate_span(1); response[0] = format(response_arena, @@ -733,12 +732,9 @@ bool HttpHandler::process_sequence_batch(BatchType &batch) { if (!status_entry.status_request_id.empty()) { // Add request_id to banned list - store the string in arena and // use string_view - char *arena_chars = banned_request_arena.allocate( - status_entry.status_request_id.size()); - std::memcpy(arena_chars, status_entry.status_request_id.data(), - status_entry.status_request_id.size()); - std::string_view request_id_view( - arena_chars, status_entry.status_request_id.size()); + std::string_view request_id_view = + banned_request_arena.copy_string( + status_entry.status_request_id); banned_request_ids.insert(request_id_view); // Update memory usage metric diff --git a/src/metric.cpp b/src/metric.cpp index 7e90b01..e8a4ca9 100644 --- a/src/metric.cpp +++ b/src/metric.cpp @@ -123,16 +123,6 @@ static void validate_or_abort(bool condition, const char *message, } } -// Helper to copy a string into arena memory -static std::string_view arena_copy_string(std::string_view str, Arena &arena) { - if (str.empty()) { - return std::string_view{}; - } - char *copied = arena.allocate(str.size()); - std::memcpy(copied, str.data(), str.size()); - return std::string_view(copied, str.size()); -} - // Arena-based labels key for second level of map // Uses string_view containing labels in Prometheus text format struct LabelsKey { @@ -149,8 +139,8 @@ struct LabelsKey { validate_or_abort(is_valid_label_value(value), "invalid label value", value); - auto key_view = arena_copy_string(key, arena); - auto value_view = arena_copy_string(value, arena); + auto key_view = arena.copy_string(key); + auto value_view = arena.copy_string(value); labels.push_back({key_view, value_view}); } @@ -581,7 +571,7 @@ struct Metric { } // Not found - copy to global arena and intern - auto interned_text = arena_copy_string(text, get_global_arena()); + auto interned_text = get_global_arena().copy_string(text); auto result = interned_set.emplace(interned_text); return *result.first; } @@ -1461,12 +1451,12 @@ Family create_counter(std::string_view name, std::string_view help) { std::unique_lock _{Metric::mutex}; ++Metric::registration_version; auto &global_arena = Metric::get_global_arena(); - auto name_view = arena_copy_string(name, global_arena); + auto name_view = global_arena.copy_string(name); auto &familyPtr = Metric::get_counter_families()[name_view]; if (!familyPtr) { familyPtr = global_arena.construct::State>(global_arena); familyPtr->name = name_view; - familyPtr->help = arena_copy_string(help, global_arena); + familyPtr->help = global_arena.copy_string(help); } else { validate_or_abort( familyPtr->help == help, @@ -1483,13 +1473,13 @@ Family create_gauge(std::string_view name, std::string_view help) { std::unique_lock _{Metric::mutex}; ++Metric::registration_version; auto &global_arena = Metric::get_global_arena(); - auto name_view = arena_copy_string(name, global_arena); + auto name_view = global_arena.copy_string(name); auto &familyPtr = Metric::get_gauge_families()[name_view]; if (!familyPtr) { // Family::State instances use Arena::Ptr for automatic cleanup familyPtr = global_arena.construct::State>(global_arena); familyPtr->name = name_view; - familyPtr->help = arena_copy_string(help, global_arena); + familyPtr->help = global_arena.copy_string(help); } else { validate_or_abort( familyPtr->help == help, @@ -1507,13 +1497,13 @@ Family create_histogram(std::string_view name, std::string_view help, std::unique_lock _{Metric::mutex}; ++Metric::registration_version; auto &global_arena = Metric::get_global_arena(); - auto name_view = arena_copy_string(name, global_arena); + auto name_view = global_arena.copy_string(name); auto &family_ptr = Metric::get_histogram_families()[name_view]; if (!family_ptr) { // Family::State instances use Arena::Ptr for automatic cleanup family_ptr = global_arena.construct::State>(global_arena); family_ptr->name = name_view; - family_ptr->help = arena_copy_string(help, global_arena); + family_ptr->help = global_arena.copy_string(help); // DESIGN: Prometheus-compatible histogram buckets // Convert to vector for sorting diff --git a/style.md b/style.md index f244e56..568c41b 100644 --- a/style.md +++ b/style.md @@ -396,6 +396,27 @@ operations = {}; // Default construct - clear() won't work correctly arena.reset(); // Reset arena memory ``` +### Arena String Copying + +- **Always use `Arena::copy_string()`** for copying string data into arena memory +- **Avoid manual allocation and memcpy** for string copying +- **Use `Arena::allocate_span()`** for array allocations instead of manual span construction + +```cpp +// Preferred - unified arena methods +std::string_view copy = arena.copy_string(original_string); +auto buffer = arena.allocate_span(1024); +auto strings = arena.allocate_span(count); + +// Avoid - manual allocation and copying +char *copied = arena.allocate(str.size()); +std::memcpy(copied, str.data(), str.size()); +std::string_view copy(copied, str.size()); + +// Avoid - manual span construction +auto span = std::span{arena.allocate(count), count}; +``` + ### Resource Management - **RAII** everywhere - constructors acquire, destructors release diff --git a/tests/test_metric.cpp b/tests/test_metric.cpp index 43458f1..0d2187c 100644 --- a/tests/test_metric.cpp +++ b/tests/test_metric.cpp @@ -585,32 +585,6 @@ TEST_CASE("thread counter cleanup bug") { } } -TEST_CASE("error conditions") { - SUBCASE("counter negative increment") { - auto counter_family = metric::create_counter("error_counter", "Error test"); - auto counter = counter_family.create({}); - - // This should abort in debug builds due to validation - // In release builds, behavior is undefined - // counter.inc(-1.0); // Would abort - } - - SUBCASE("invalid metric names") { - // These should abort due to validation - // auto bad_counter = metric::create_counter("123invalid", "help"); // Would - // abort auto bad_gauge = metric::create_gauge("invalid-name", "help"); // - // Would abort - } - - SUBCASE("invalid label keys") { - auto counter_family = metric::create_counter("valid_name", "help"); - - // This should abort due to label validation - // auto counter = counter_family.create({{"123invalid", "value"}}); // Would - // abort - } -} - TEST_CASE("memory management") { SUBCASE("arena allocation in render") { Arena arena; diff --git a/tests/test_server.cpp b/tests/test_server.cpp index e614a86..9f25ad6 100644 --- a/tests/test_server.cpp +++ b/tests/test_server.cpp @@ -8,24 +8,14 @@ #include #include -// Helper to copy a string into arena memory -static std::string_view arena_copy_string(std::string_view str, Arena &arena) { - if (str.empty()) { - return std::string_view{}; - } - char *copied = arena.allocate(str.size()); - std::memcpy(copied, str.data(), str.size()); - return std::string_view(copied, str.size()); -} - struct EchoHandler : ConnectionHandler { Arena arena; std::span reply; WeakRef wconn; std::latch done{1}; void on_data_arrived(std::string_view data, Connection &conn) override { - reply = std::span{arena.allocate(1), 1}; - reply[0] = arena_copy_string(data, arena); + reply = arena.allocate_span(1); + reply[0] = arena.copy_string(data); wconn = conn.get_weak_ref(); CHECK(wconn.lock()); done.count_down();