This is an automated email from the ASF dual-hosted git repository.
wkaras pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 46a6570f63 Cleanup of HdrHeap::HeapGuard and HdrStrHeap classes.
(#10961)
46a6570f63 is described below
commit 46a6570f6361f354aed486ea6f1ca98b7aad64ee
Author: Walt Karas <[email protected]>
AuthorDate: Tue Jan 9 15:33:05 2024 -0500
Cleanup of HdrHeap::HeapGuard and HdrStrHeap classes. (#10961)
* Cleanup of HdrStrHeap class.
* Encapsulate HdrHeap::HeapGuard.
---
include/proxy/hdrs/HdrHeap.h | 37 ++++++---
src/proxy/hdrs/HTTP.cc | 2 +-
src/proxy/hdrs/HdrHeap.cc | 124 ++++++++++++++++--------------
src/proxy/hdrs/unit_tests/test_HdrHeap.cc | 10 +--
4 files changed, 97 insertions(+), 76 deletions(-)
diff --git a/include/proxy/hdrs/HdrHeap.h b/include/proxy/hdrs/HdrHeap.h
index d2bd2b6b0c..f2a5cb335b 100644
--- a/include/proxy/hdrs/HdrHeap.h
+++ b/include/proxy/hdrs/HdrHeap.h
@@ -136,19 +136,32 @@ public:
char *allocate(int nbytes);
char *expand(char *ptr, int old_size, int new_size);
- int space_avail();
-
- uint32_t m_heap_size;
- char *m_free_start;
- uint32_t m_free_size;
+ uint32_t
+ space_avail() const
+ {
+ return _avail_size;
+ }
+ uint32_t
+ total_size() const
+ {
+ return _total_size;
+ }
bool contains(const char *str) const;
+
+ static HdrStrHeap *alloc(int heap_size);
+
+private:
+ HdrStrHeap(uint32_t total_size) : _total_size{total_size} {}
+
+ uint32_t const _total_size;
+ uint32_t _avail_size;
};
inline bool
HdrStrHeap::contains(const char *str) const
{
- return reinterpret_cast<char const *>(this + 1) <= str && str <
reinterpret_cast<char const *>(this) + m_heap_size;
+ return reinterpret_cast<char const *>(this + 1) <= str && str <
reinterpret_cast<char const *>(this) + _total_size;
}
struct StrHeapDesc {
@@ -289,16 +302,18 @@ public:
the reference is dropped. This is useful inside a method or block
to keep the required heap data around until leaving the scope.
*/
- struct HeapGuard {
+ class HeapGuard
+ {
+ public:
/// Construct the protection.
HeapGuard(HdrHeap *heap, const char *str)
{
if (heap->m_read_write_heap && heap->m_read_write_heap->contains(str)) {
- m_ptr = heap->m_read_write_heap.get();
+ _ptr = heap->m_read_write_heap.get();
} else {
for (auto &i : heap->m_ronly_heap) {
if (i.contains(str)) {
- m_ptr = i.m_ref_count_ptr;
+ _ptr = i.m_ref_count_ptr;
break;
}
}
@@ -308,8 +323,9 @@ public:
// There's no need to have a destructor here, the default dtor will take
care of
// releasing the (potentially) locked heap.
+ private:
/// The heap we protect (if any)
- Ptr<RefCountObj> m_ptr;
+ Ptr<RefCountObj> _ptr;
};
// String Heap access
@@ -497,7 +513,6 @@ HdrHeapSDKHandle::set(const HdrHeapSDKHandle *from)
m_heap = from->m_heap;
}
-HdrStrHeap *new_HdrStrHeap(int requested_size);
HdrHeap *new_HdrHeap(int size = HdrHeap::DEFAULT_SIZE);
void hdr_heap_test();
diff --git a/src/proxy/hdrs/HTTP.cc b/src/proxy/hdrs/HTTP.cc
index c260e5d7f8..07feb22bf0 100644
--- a/src/proxy/hdrs/HTTP.cc
+++ b/src/proxy/hdrs/HTTP.cc
@@ -733,7 +733,7 @@ http_hdr_url_set(HdrHeap *heap, HTTPHdrImpl *hh, URLImpl
*url)
// Make sure there is a read_write heap
if (heap->m_read_write_heap.get() == nullptr) {
int url_string_length = url->strings_length();
- heap->m_read_write_heap = new_HdrStrHeap(url_string_length);
+ heap->m_read_write_heap = HdrStrHeap::alloc(url_string_length);
}
hh->u.req.m_url_impl->rehome_strings(heap);
} else {
diff --git a/src/proxy/hdrs/HdrHeap.cc b/src/proxy/hdrs/HdrHeap.cc
index ccfb44caa8..2b137fe57c 100644
--- a/src/proxy/hdrs/HdrHeap.cc
+++ b/src/proxy/hdrs/HdrHeap.cc
@@ -31,6 +31,7 @@
****************************************************************************/
#include "tscore/ink_platform.h"
+#include "tscore/Diags.h"
#include "proxy/hdrs/HdrHeap.h"
#include "proxy/hdrs/URL.h"
#include "proxy/hdrs/MIME.h"
@@ -124,14 +125,14 @@ new_HdrHeap(int size)
}
HdrStrHeap *
-new_HdrStrHeap(int requested_size)
+HdrStrHeap::alloc(int heap_size)
{
// The callee is asking for a string heap to be created
// that can allocate at least size bytes. As such we,
// need to include the size of the string heap header in
// our calculations
- int alloc_size = requested_size + sizeof(HdrStrHeap);
+ int alloc_size = heap_size + sizeof(HdrStrHeap);
HdrStrHeap *sh;
if (alloc_size <= HdrStrHeap::DEFAULT_SIZE) {
@@ -142,18 +143,16 @@ new_HdrStrHeap(int requested_size)
sh = static_cast<HdrStrHeap *>(ats_malloc(alloc_size));
}
- // Debug("hdrs", "Allocated string heap in size %d", alloc_size);
-
// Placement new the HdrStrHeap.
- sh = new (sh) HdrStrHeap();
+ sh = new (sh) HdrStrHeap(alloc_size);
- sh->m_heap_size = alloc_size;
- sh->m_free_size = alloc_size - sizeof(HdrStrHeap);
- sh->m_free_start = reinterpret_cast<char *>(sh + 1);
+ sh->_avail_size = alloc_size - sizeof(HdrStrHeap);
ink_assert(sh->refcount() == 0);
- ink_assert(sh->m_free_size > 0);
+ ink_assert(int(sh->total_size()) == alloc_size);
+
+ ink_assert(sh->_avail_size > 0);
return sh;
}
@@ -229,8 +228,8 @@ HdrHeap::deallocate_obj(HdrHeapObjImpl *obj)
char *
HdrHeap::allocate_str(int nbytes)
{
- int last_size = 0;
- char *new_space = nullptr;
+ int last_size = 0;
+ int next_size = 0;
ink_assert(m_writeable);
// INKqa08287 - We could get infinite build up
@@ -241,39 +240,50 @@ HdrHeap::allocate_str(int nbytes)
// but I already no that this code path is
// safe for forcing a str coalesce so I'm doing
// it here for sanity's sake
- if (m_lost_string_space > static_cast<int>(MAX_LOST_STR_SPACE)) {
- goto FAILED;
- }
+ int coalesce = m_lost_string_space > static_cast<int>(MAX_LOST_STR_SPACE) ?
1 : 0;
-RETRY:
- // First check to see if we have a read/write
- // string heap
- if (!m_read_write_heap) {
- int next_size = (last_size * 2) - sizeof(HdrStrHeap);
- next_size = next_size > nbytes ? next_size : nbytes;
- m_read_write_heap = new_HdrStrHeap(next_size);
- }
- // Try to allocate of our read/write string heap
- new_space = m_read_write_heap->allocate(nbytes);
+ for (;;) {
+ if (coalesce) {
+ switch (coalesce) {
+ case 2:
+ Warning("HdrHeap=%p coalescing twice", this);
+ break;
+ case 3:
+ Warning("HdrHeap=%p coalescing three or more times", this);
+ break;
+ default:
+ break;
+ }
- if (new_space) {
- return new_space;
- }
+ coalesce_str_heaps();
+ }
+ do {
+ // First check to see if we have a read/write
+ // string heap
+ if (!m_read_write_heap) {
+ if (next_size) {
+ Warning("HdrHeap=%p new read/write string heap twice last_size=%d",
this, last_size);
+ }
+ next_size = (last_size * 2) - int(sizeof(HdrStrHeap));
+ next_size = next_size > nbytes ? next_size : nbytes;
+ m_read_write_heap = HdrStrHeap::alloc(next_size);
+ }
+ // Try to allocate of our read/write string heap
+ if (char *new_space = m_read_write_heap->allocate(nbytes); new_space) {
+ return new_space;
+ }
- last_size = m_read_write_heap->m_heap_size;
+ last_size = m_read_write_heap->total_size();
- // Our existing rw str heap doesn't have sufficient
- // capacity. We need to move the current rw heap
- // out of the way and create a new one
- if (demote_rw_str_heap() == 0) {
- goto RETRY;
- }
+ // Our existing rw str heap doesn't have sufficient
+ // capacity. We need to move the current rw heap
+ // out of the way and create a new one
+ } while (demote_rw_str_heap() == 0);
-FAILED:
- // We failed to demote. We'll have to coalesce
- // the heaps
- coalesce_str_heaps();
- goto RETRY;
+ // We failed to demote. We'll have to coalesce the heaps.
+ ++coalesce;
+ next_size = 0;
+ }
}
// char* HdrHeap::expand_str(const char* old_str, int old_len, int new_len)
@@ -325,9 +335,9 @@ HdrHeap::demote_rw_str_heap()
// We've found a slot
i.m_ref_count_ptr = m_read_write_heap.object();
i.m_heap_start = reinterpret_cast<char *>(m_read_write_heap.get());
- i.m_heap_len = m_read_write_heap->m_heap_size -
m_read_write_heap->m_free_size;
+ i.m_heap_len = m_read_write_heap->total_size() -
m_read_write_heap->space_avail();
- // Debug("hdrs", "Demoted rw heap of %d size",
m_read_write_heap->m_heap_size);
+ // Debug("hdrs", "Demoted rw heap of %d size",
m_read_write_heap->total_size());
m_read_write_heap = nullptr;
return 0;
}
@@ -356,7 +366,7 @@ HdrHeap::coalesce_str_heaps(int incoming_size)
new_heap_size += required_space_for_evacuation();
- HdrStrHeap *new_heap = new_HdrStrHeap(new_heap_size);
+ HdrStrHeap *new_heap = HdrStrHeap::alloc(new_heap_size);
evacuate_from_str_heaps(new_heap);
m_lost_string_space = 0;
@@ -492,7 +502,7 @@ HdrHeap::sanity_check_strs()
if (m_read_write_heap) {
heaps[num_heaps].start = (reinterpret_cast<char
*>(m_read_write_heap.get())) + sizeof(HdrStrHeap);
- int heap_size = m_read_write_heap->m_heap_size - (sizeof(HdrStrHeap) +
m_read_write_heap->m_free_size);
+ int heap_size = m_read_write_heap->total_size() - (sizeof(HdrStrHeap) +
m_read_write_heap->space_avail());
heaps[num_heaps].end = heaps[num_heaps].start + heap_size;
num_heaps++;
@@ -572,7 +582,7 @@ HdrHeap::marshal_length()
// heap, we can drop the header on the read/write
// string heap
if (m_read_write_heap) {
- len += m_read_write_heap->m_heap_size - (sizeof(HdrStrHeap) +
m_read_write_heap->m_free_size);
+ len += m_read_write_heap->total_size() - (sizeof(HdrStrHeap) +
m_read_write_heap->space_avail());
}
for (auto &j : m_ronly_heap) {
@@ -707,7 +717,7 @@ HdrHeap::marshal(char *buf, int len)
if (m_read_write_heap) {
char *copy_start = (reinterpret_cast<char *>(m_read_write_heap.get())) +
sizeof(HdrStrHeap);
- int nto_copy = m_read_write_heap->m_heap_size - (sizeof(HdrStrHeap) +
m_read_write_heap->m_free_size);
+ int nto_copy = m_read_write_heap->total_size() - (sizeof(HdrStrHeap) +
m_read_write_heap->space_avail());
if (nto_copy > len) {
goto Failed;
@@ -1044,7 +1054,7 @@ HdrHeap::inherit_string_heaps(const HdrHeap *inherit_from)
// Find out if we have enough slots
if (inherit_from->m_read_write_heap) {
free_slots--;
- inherit_str_size = inherit_from->m_read_write_heap->m_heap_size;
+ inherit_str_size = inherit_from->m_read_write_heap->total_size();
}
for (const auto &index : inherit_from->m_ronly_heap) {
if (index.m_heap_start != nullptr) {
@@ -1074,7 +1084,7 @@ HdrHeap::inherit_string_heaps(const HdrHeap *inherit_from)
// Copy over read/write string heap if it exists
if (inherit_from->m_read_write_heap) {
int str_size =
- inherit_from->m_read_write_heap->m_heap_size - sizeof(HdrStrHeap) -
inherit_from->m_read_write_heap->m_free_size;
+ inherit_from->m_read_write_heap->total_size() - sizeof(HdrStrHeap) -
inherit_from->m_read_write_heap->space_avail();
ink_release_assert(attach_str_heap(reinterpret_cast<char
*>(inherit_from->m_read_write_heap.get() + 1), str_size,
inherit_from->m_read_write_heap.get(), &first_free));
}
@@ -1153,7 +1163,7 @@ HdrHeap::total_used_size() const
void
HdrStrHeap::free()
{
- if (m_heap_size == HdrStrHeap::DEFAULT_SIZE) {
+ if (_total_size == HdrStrHeap::DEFAULT_SIZE) {
THREAD_FREE(this, strHeapAllocator, this_thread());
} else {
ats_free(this);
@@ -1168,12 +1178,9 @@ HdrStrHeap::free()
char *
HdrStrHeap::allocate(int nbytes)
{
- char *new_space;
-
- if (m_free_size >= static_cast<unsigned>(nbytes)) {
- new_space = m_free_start;
- m_free_start += nbytes;
- m_free_size -= nbytes;
+ if (_avail_size >= static_cast<unsigned>(nbytes)) {
+ char *new_space = reinterpret_cast<char *>(this) + _total_size -
_avail_size;
+ _avail_size -= nbytes;
return new_space;
} else {
return nullptr;
@@ -1190,12 +1197,11 @@ HdrStrHeap::expand(char *ptr, int old_size, int
new_size)
{
unsigned int expand_size = new_size - old_size;
- ink_assert(ptr >= reinterpret_cast<char const *>(this + 1));
- ink_assert(ptr < reinterpret_cast<char const *>(this) + m_heap_size);
+ ink_assert(contains(ptr));
- if (ptr + old_size == m_free_start && expand_size <= m_free_size) {
- m_free_start += expand_size;
- m_free_size -= expand_size;
+ char *free_start = reinterpret_cast<char *>(this) + _total_size -
_avail_size;
+ if (ptr + old_size == free_start && expand_size <= _avail_size) {
+ _avail_size -= expand_size;
return ptr;
} else {
return nullptr;
diff --git a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
index 3aca536706..1060be5b0e 100644
--- a/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
+++ b/src/proxy/hdrs/unit_tests/test_HdrHeap.cc
@@ -45,7 +45,7 @@ TEST_CASE("HdrHeap", "[proxy][hdrheap]")
url->set_path(heap, buf, next_required_overflow_size, true);
// Checking that we've completely consumed the rw heap
- CHECK(heap->m_read_write_heap->m_free_size == 0);
+ CHECK(heap->m_read_write_heap->space_avail() == 0);
// Checking ronly_heaps are empty
for (unsigned i = 0; i < HDR_BUF_RONLY_HEAPS; ++i) {
@@ -55,7 +55,7 @@ TEST_CASE("HdrHeap", "[proxy][hdrheap]")
// Now we have no ronly heaps in use and a completely full rwheap, so we
will test that
// we demote to ronly heaps HDR_BUF_RONLY_HEAPS times.
for (unsigned ronly_heap = 0; ronly_heap < HDR_BUF_RONLY_HEAPS;
++ronly_heap) {
- next_rw_heap_size = 2 * heap->m_read_write_heap->m_heap_size;
+ next_rw_heap_size = 2 * heap->m_read_write_heap->total_size();
next_required_overflow_size = next_rw_heap_size - sizeof(HdrStrHeap);
char buf2[next_required_overflow_size];
for (unsigned int i = 0; i < sizeof(buf2); ++i) {
@@ -66,9 +66,9 @@ TEST_CASE("HdrHeap", "[proxy][hdrheap]")
url2->set_path(heap, buf2, next_required_overflow_size, true);
// Checking the current rw heap is next_rw_heap_size bytes
- CHECK(heap->m_read_write_heap->m_heap_size == (uint32_t)next_rw_heap_size);
+ CHECK(heap->m_read_write_heap->total_size() ==
(uint32_t)next_rw_heap_size);
// Checking that we've completely consumed the rw heap
- CHECK(heap->m_read_write_heap->m_free_size == 0);
+ CHECK(heap->m_read_write_heap->space_avail() == 0);
// Checking that we properly demoted the previous rw heap
CHECK(heap->m_ronly_heap[ronly_heap].m_heap_start != nullptr);
@@ -103,7 +103,7 @@ TEST_CASE("HdrHeap", "[proxy][hdrheap]")
CHECK(heap->m_ronly_heap[i].m_heap_start != nullptr);
}
// Checking that we've completely consumed the rw heap
- CHECK(heap->m_read_write_heap->m_free_size == 0);
+ CHECK(heap->m_read_write_heap->space_avail() == 0);
// Checking that we dont have any chained heaps
CHECK(heap->m_next == nullptr);
