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 <wka...@yahooinc.com> 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);