Modernize BlockAllocator and its utility functions

src/CMakeLists.txt

@@ -194,6 +194,7 @@ if(ENABLE_APP)
       template_test.cc
       base64_test.cc
       network_test.cc
+      allocator_test.cc
       ${CMAKE_SOURCE_DIR}/tests/munit/munit.c
     )
     if(ENABLE_HTTP3)

src/Makefile.am

@@ -232,6 +232,7 @@ nghttpx_unittest_SOURCES = shrpx-unittest.cc \
 	template_test.cc template_test.h \
 	base64_test.cc base64_test.h \
 	network_test.cc network_test.h \
+	allocator_test.cc allocator_test.h \
 	$(top_srcdir)/tests/munit/munit.c $(top_srcdir)/tests/munit/munit.h \
 	$(top_srcdir)/tests/munit/munitxx.h
 if ENABLE_HTTP3

src/allocator.h

@@ -71,10 +71,8 @@ static_assert(sizeof(ChunkHead) == ALIGNMENT);
 // |block_size|.
 struct BlockAllocator {
   BlockAllocator(size_t block_size, size_t isolation_threshold)
-    : retain(nullptr),
-      head(nullptr),
-      block_size(block_size),
-      isolation_threshold(std::min(block_size, isolation_threshold)) {
+    : block_size{block_size},
+      isolation_threshold{std::min(block_size, isolation_threshold)} {
     assert(isolation_threshold <= block_size);
   }
 
@@ -83,10 +81,14 @@ struct BlockAllocator {
   BlockAllocator(BlockAllocator &&other) noexcept
     : retain{std::exchange(other.retain, nullptr)},
       head{std::exchange(other.head, nullptr)},
-      block_size(other.block_size),
-      isolation_threshold(other.isolation_threshold) {}
+      block_size{other.block_size},
+      isolation_threshold{other.isolation_threshold} {}
 
   BlockAllocator &operator=(BlockAllocator &&other) noexcept {
+    if (this == &other) {
+      return *this;
+    }
+
     reset();
 
     retain = std::exchange(other.retain, nullptr);
@@ -123,22 +125,22 @@ struct BlockAllocator {
     };
 
     retain = mb;
+
     return mb;
   }
 
   constexpr size_t alloc_unit(size_t size) { return sizeof(ChunkHead) + size; }
 
-  void *alloc(size_t size) {
+  std::span<uint8_t> alloc(size_t size) {
     auto au = alloc_unit(size);
 
     if (au >= isolation_threshold) {
-      size = std::max(ALIGNMENT, size);
       // We will store the allocated size in size_t field.
       auto mb = alloc_mem_block(alloc_unit(size));
-      auto ch = reinterpret_cast<ChunkHead *>(mb->begin);
+      auto ch = new (mb->begin) ChunkHead{};
       ch->size = size;
       mb->last = mb->end;
-      return mb->begin + sizeof(ChunkHead);
+      return {mb->begin + sizeof(ChunkHead), size};
     }
 
     if (!head || static_cast<size_t>(head->end - head->last) < au) {
@@ -160,46 +162,46 @@ struct BlockAllocator {
       head->last = head->end;
     }
 
-    return res;
+    return {res, size};
   }
 
   // Returns allocated size for memory pointed by |ptr|.  We assume
   // that |ptr| was returned from alloc() or realloc().
-  size_t get_alloc_length(void *ptr) {
-    return reinterpret_cast<ChunkHead *>(static_cast<uint8_t *>(ptr) -
-                                         sizeof(ChunkHead))
-      ->size;
+  size_t get_alloc_length(const uint8_t *ptr) {
+    return reinterpret_cast<const ChunkHead *>(ptr - sizeof(ChunkHead))->size;
   }
 
   // Allocates memory of at least |size| bytes.  If |ptr| is nullptr,
   // this is equivalent to alloc(size).  If |ptr| is not nullptr,
   // obtain the allocated size for |ptr|, assuming that |ptr| was
   // returned from alloc() or realloc().  If the allocated size is
-  // greater than or equal to size, |ptr| is returned.  Otherwise,
-  // allocates at least |size| bytes of memory, and the original
-  // content pointed by |ptr| is copied to the newly allocated memory.
-  void *realloc(void *ptr, size_t size) {
+  // greater than or equal to size, std::span{|ptr|, |size|} is
+  // returned.  Otherwise, allocates at least |size| bytes of memory,
+  // and the original content pointed by |ptr| is copied to the newly
+  // allocated memory, and returns the std::span{p, |size|}, where p
+  // is the pointer to the allocated memory.
+  std::span<uint8_t> realloc(const uint8_t *ptr, size_t size) {
     if (!ptr) {
       return alloc(size);
     }
+
     auto alloclen = get_alloc_length(ptr);
-    auto p = reinterpret_cast<uint8_t *>(ptr);
     if (size <= alloclen) {
-      return ptr;
+      return {const_cast<uint8_t *>(ptr), size};
     }
 
-    auto nalloclen = std::max(size + 1, alloclen * 2);
-
+    auto nalloclen = std::max(size, alloclen * 2);
     auto res = alloc(nalloclen);
-    std::ranges::copy_n(p, as_signed(alloclen), static_cast<uint8_t *>(res));
 
-    return res;
+    std::ranges::copy(std::span{ptr, alloclen}, std::ranges::begin(res));
+
+    return res.first(size);
   }
 
   // This holds live memory block to free them in dtor.
-  MemBlock *retain;
+  MemBlock *retain{};
   // Current memory block to use.
-  MemBlock *head;
+  MemBlock *head{};
   // size of single memory block
   size_t block_size;
   // if allocation greater or equal to isolation_threshold bytes is
@@ -211,12 +213,11 @@ struct BlockAllocator {
 // will be NULL-terminated.
 template <std::forward_iterator I>
 std::string_view make_string_ref(BlockAllocator &alloc, I first, I last) {
-  auto dst = static_cast<char *>(
-    alloc.alloc(static_cast<size_t>(std::ranges::distance(first, last) + 1)));
-  auto p = std::ranges::copy(first, last, dst).out;
-  *p = '\0';
+  auto len = as_unsigned(std::ranges::distance(first, last));
+  auto res = alloc.alloc(len + 1);
+  *std::ranges::copy(first, last, std::ranges::begin(res)).out = '\0';
 
-  return std::string_view{dst, p};
+  return as_string_view(res.first(len));
 }
 
 // Makes a copy of |r| as std::string_view.  The resulting string will be
@@ -242,17 +243,18 @@ constexpr size_t concat_string_ref_count(size_t acc, R &&r, Args &&...args) {
 
 // private function used in concat_string_ref.  this is the base
 // function of concat_string_ref_copy().
-inline uint8_t *concat_string_ref_copy(uint8_t *p) { return p; }
+inline constexpr void concat_string_ref_copy(std::span<uint8_t> dst) {}
 
 // private function used in concat_string_ref.  This function copies
-// given strings into |p|.  |p| is incremented by the copied length,
-// and returned.  In the end, return value points to the location one
-// beyond the last byte written.
-template <std::ranges::input_range R, std::ranges::input_range... Args>
+// given strings into |dst|.
+template <std::ranges::sized_range R, std::ranges::sized_range... Args>
 requires(!std::is_array_v<std::remove_cvref_t<R>>)
-uint8_t *concat_string_ref_copy(uint8_t *p, R &&r, Args &&...args) {
-  return concat_string_ref_copy(std::ranges::copy(std::forward<R>(r), p).out,
-                                std::forward<Args>(args)...);
+constexpr void concat_string_ref_copy(std::span<uint8_t> dst, R &&r,
+                                      Args &&...args) {
+  concat_string_ref_copy(
+    {std::ranges::copy(std::forward<R>(r), std::ranges::begin(dst)).out,
+     std::ranges::end(dst)},
+    std::forward<Args>(args)...);
 }
 
 // Returns the string which is the concatenation of |args| in the
@@ -260,19 +262,20 @@ uint8_t *concat_string_ref_copy(uint8_t *p, R &&r, Args &&...args) {
 template <std::ranges::sized_range... Args>
 std::string_view concat_string_ref(BlockAllocator &alloc, Args &&...args) {
   auto len = concat_string_ref_count(0, args...);
-  auto dst = static_cast<uint8_t *>(alloc.alloc(len + 1));
-  auto p = dst;
-  p = concat_string_ref_copy(p, std::forward<Args>(args)...);
-  *p = '\0';
-  return as_string_view(dst, p);
+  auto res = alloc.alloc(len + 1);
+
+  concat_string_ref_copy(res, std::forward<Args>(args)...);
+  res.back() = '\0';
+
+  return as_string_view(res.first(len));
 }
 
 // Returns the string which is the concatenation of |value| and |args|
 // in the given order.  The resulting string will be NULL-terminated.
-// This function assumes that the pointer value value.c_str() was
-// obtained from alloc.alloc() or alloc.realloc(), and attempts to use
-// unused memory region by using alloc.realloc().  If value is empty,
-// then just call concat_string_ref().
+// This function assumes that value.data() was obtained from
+// alloc.alloc() or alloc.realloc(), and attempts to use unused memory
+// region by using alloc.realloc().  If value is empty, then just call
+// concat_string_ref().
 template <std::ranges::sized_range... Args>
 std::string_view realloc_concat_string_ref(BlockAllocator &alloc,
                                            std::string_view value,
@@ -282,19 +285,18 @@ std::string_view realloc_concat_string_ref(BlockAllocator &alloc,
   }
 
   auto len = value.size() + concat_string_ref_count(0, args...);
-  auto dst = static_cast<uint8_t *>(alloc.realloc(
-    const_cast<uint8_t *>(reinterpret_cast<const uint8_t *>(value.data())),
-    len + 1));
-  auto p = dst + value.size();
-  p = concat_string_ref_copy(p, std::forward<Args>(args)...);
-  *p = '\0';
+  auto res =
+    alloc.realloc(reinterpret_cast<const uint8_t *>(value.data()), len + 1);
+  concat_string_ref_copy(res.subspan(value.size()),
+                         std::forward<Args>(args)...);
+  res.back() = '\0';
 
-  return as_string_view(dst, p);
+  return as_string_view(res.first(len));
 }
 
 // Makes an uninitialized buffer with given size.
 inline std::span<uint8_t> make_byte_ref(BlockAllocator &alloc, size_t size) {
-  return {static_cast<uint8_t *>(alloc.alloc(size)), size};
+  return alloc.alloc(size);
 }
 
 } // namespace nghttp2

src/allocator_test.cc

@@ -0,0 +1,190 @@
+/*
+ * nghttp2 - HTTP/2 C Library
+ *
+ * Copyright (c) 2026 nghttp2 contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+#include "allocator_test.h"
+
+#include "munitxx.h"
+
+#include <nghttp2/nghttp2.h>
+
+#include "util.h"
+#include "template.h"
+
+using namespace std::literals;
+
+namespace nghttp2 {
+
+namespace {
+const MunitTest tests[]{
+  munit_void_test(test_allocator_alloc),
+  munit_void_test(test_allocator_realloc),
+  munit_void_test(test_make_string_ref),
+  munit_void_test(test_concat_string_ref),
+  munit_void_test(test_realloc_concat_string_ref),
+  munit_test_end(),
+};
+} // namespace
+
+const MunitSuite allocator_suite{
+  "/allocator", tests, nullptr, 1, MUNIT_SUITE_OPTION_NONE,
+};
+
+namespace {
+auto data = []() {
+  std::array<uint8_t, 4096> data;
+
+  std::ranges::fill(data, 0xfe);
+
+  return data;
+}();
+} // namespace
+
+void test_allocator_alloc(void) {
+  {
+    BlockAllocator balloc{4096, 1024};
+
+    auto p = balloc.alloc(117);
+
+    assert_size(117, ==, std::ranges::size(p));
+
+    // Check p is writable
+    std::ranges::copy(std::span{data}.first(std::ranges::size(p)),
+                      std::ranges::begin(p));
+
+    assert_memory_equal(std::ranges::size(p), std::ranges::data(data),
+                        std::ranges::data(p));
+
+    // Check the isolation threshold works.
+    assert_ptr_equal(balloc.retain, balloc.head);
+
+    p = balloc.alloc(1024);
+
+    std::ranges::copy(std::span{data}.first(std::ranges::size(p)),
+                      std::ranges::begin(p));
+
+    assert_memory_equal(std::ranges::size(p), std::ranges::data(data),
+                        std::ranges::data(p));
+    assert_ptr_not_equal(balloc.retain, balloc.head);
+  }
+
+  {
+    BlockAllocator balloc{32, 32};
+
+    // This consumes the allocated block.
+    auto p = balloc.alloc(8);
+
+    assert_size(8, ==, std::ranges::size(p));
+    assert_ptr_equal(balloc.head->last, balloc.head->end);
+
+    // This allocates new block.
+    p = balloc.alloc(8);
+
+    assert_not_null(balloc.retain->next);
+  }
+}
+
+void test_allocator_realloc(void) {
+  {
+    BlockAllocator balloc{4096, 1024};
+
+    constexpr size_t alloclen = 100;
+
+    auto p = balloc.alloc(alloclen);
+    auto orig_ptr = std::ranges::data(p);
+
+    p = balloc.realloc(std::ranges::data(p), 110);
+
+    assert_ptr_not_equal(orig_ptr, std::ranges::data(p));
+    assert_size(200, ==, balloc.get_alloc_length(std::ranges::data(p)));
+    assert_size(110, ==, std::ranges::size(p));
+  }
+
+  {
+    BlockAllocator balloc{4096, 1024};
+
+    auto p = balloc.alloc(100);
+    auto orig_ptr = std::ranges::data(p);
+
+    p = balloc.realloc(std::ranges::data(p), 100);
+
+    assert_ptr_equal(orig_ptr, std::ranges::data(p));
+    assert_size(100, ==, std::ranges::size(p));
+  }
+}
+
+void test_make_string_ref(void) {
+  BlockAllocator balloc{256, 256};
+
+  auto s = make_string_ref(balloc, "foo the bar"sv);
+
+  assert_stdsv_equal("foo the bar"sv, s);
+}
+
+void test_concat_string_ref(void) {
+  BlockAllocator balloc{256, 256};
+
+  auto s = concat_string_ref(balloc, "alpha "sv, "bravo "sv, "charlie"sv);
+
+  assert_stdsv_equal("alpha bravo charlie"sv, s);
+}
+
+void test_realloc_concat_string_ref(void) {
+  BlockAllocator balloc{256, 256};
+
+  auto s = make_string_ref(balloc, "alpha"sv);
+  assert_size(6, ==,
+              balloc.get_alloc_length(
+                reinterpret_cast<const uint8_t *>(std::ranges::data(s))));
+
+  auto t = realloc_concat_string_ref(balloc, s, " "sv, "bravo"sv);
+
+  assert_stdsv_equal("alpha bravo"sv, t);
+  assert_ptr_not_equal(std::ranges::data(s), std::ranges::data(t));
+  assert_size(12, ==,
+              balloc.get_alloc_length(
+                reinterpret_cast<const uint8_t *>(std::ranges::data(t))));
+
+  auto u = realloc_concat_string_ref(balloc, t, " charlie"sv);
+
+  assert_stdsv_equal("alpha bravo charlie"sv, u);
+  assert_ptr_not_equal(std::ranges::data(t), std::ranges::data(u));
+  assert_size(24, ==,
+              balloc.get_alloc_length(
+                reinterpret_cast<const uint8_t *>(std::ranges::data(u))));
+
+  auto v = realloc_concat_string_ref(balloc, u, " delta"sv);
+
+  assert_stdsv_equal("alpha bravo charlie delta"sv, v);
+  assert_ptr_not_equal(std::ranges::data(u), std::ranges::data(v));
+  assert_size(48, ==,
+              balloc.get_alloc_length(
+                reinterpret_cast<const uint8_t *>(std::ranges::data(v))));
+
+  auto w = realloc_concat_string_ref(balloc, v, " echo"sv);
+
+  assert_stdsv_equal("alpha bravo charlie delta echo"sv, w);
+  assert_ptr_equal(std::ranges::data(v), std::ranges::data(w));
+}
+
+} // namespace nghttp2

src/allocator_test.h

@@ -0,0 +1,48 @@
+/*
+ * nghttp2 - HTTP/2 C Library
+ *
+ * Copyright (c) 2026 nghttp2 contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+#ifndef ALLOCATOR_TEST_H
+#define ALLOCATOR_TEST_H
+
+#ifdef HAVE_CONFIG_H
+#  include <config.h>
+#endif // defined(HAVE_CONFIG_H)
+
+#define MUNIT_ENABLE_ASSERT_ALIASES
+
+#include "munit.h"
+
+namespace nghttp2 {
+
+extern const MunitSuite allocator_suite;
+
+munit_void_test_decl(test_allocator_alloc)
+munit_void_test_decl(test_allocator_realloc)
+munit_void_test_decl(test_make_string_ref)
+munit_void_test_decl(test_concat_string_ref)
+munit_void_test_decl(test_realloc_concat_string_ref)
+
+} // namespace nghttp2
+
+#endif // !defined(ALLOCATOR_TEST_H)

src/shrpx-unittest.cc

@@ -49,29 +49,23 @@
 #ifdef ENABLE_HTTP3
 #  include "siphash_test.h"
 #endif // defined(ENABLE_HTTP3)
+#include "allocator_test.h"
 
 int main(int argc, char *argv[]) {
   shrpx::create_config();
 
   const MunitSuite suites[] = {
-    shrpx::tls_suite,
-    shrpx::downstream_suite,
-    shrpx::config_suite,
-    shrpx::worker_suite,
-    shrpx::http_suite,
-    shrpx::router_suite,
-    shrpx::http2_suite,
-    shrpx::util_suite,
-    gzip_suite,
-    buffer_suite,
-    memchunk_suite,
-    template_suite,
-    base64_suite,
-    network_suite,
+    shrpx::tls_suite,    shrpx::downstream_suite,
+    shrpx::config_suite, shrpx::worker_suite,
+    shrpx::http_suite,   shrpx::router_suite,
+    shrpx::http2_suite,  shrpx::util_suite,
+    gzip_suite,          buffer_suite,
+    memchunk_suite,      template_suite,
+    base64_suite,        network_suite,
 #ifdef ENABLE_HTTP3
     siphash_suite,
 #endif // defined(ENABLE_HTTP3)
-    {},
+    allocator_suite,     {},
   };
   const MunitSuite suite = {
     "", nullptr, suites, 1, MUNIT_SUITE_OPTION_NONE,