Skip to content

Commit

Permalink
Cleanup of HdrHeap::HeapGuard and HdrStrHeap classes. (apache#10961)
Browse files Browse the repository at this point in the history 
* Cleanup of HdrStrHeap class.

* Encapsulate HdrHeap::HeapGuard.
  • Loading branch information
@ywkaras
ywkaras authored Jan 9, 2024
1 parent b4ae89e commit 46a6570
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 76 deletions.
37 changes: 26 additions & 11 deletions include/proxy/hdrs/HdrHeap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,32 @@ class HdrStrHeap : public RefCountObj

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 {
Expand Down Expand Up @@ -289,16 +302,18 @@ class HdrHeap
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;
}
}
Expand All @@ -308,8 +323,9 @@ class HdrHeap
// 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
Expand Down Expand Up @@ -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();
2 changes: 1 addition & 1 deletion src/proxy/hdrs/HTTP.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
124 changes: 65 additions & 59 deletions src/proxy/hdrs/HdrHeap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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++;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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;
Expand Down
10 changes: 5 additions & 5 deletions src/proxy/hdrs/unit_tests/test_HdrHeap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 46a6570

Please sign in to comment.