You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by James Peach <jp...@apache.org> on 2014/05/12 18:17:13 UTC
Re: [1/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't
properly calculate new heap size
Brian,
Can you please add a test for this? That would help other devs understand how this is supposed to work and prevent regressions in the future
thanks!
On May 1, 2014, at 1:51 PM, briang@apache.org wrote:
> Repository: trafficserver
> Updated Branches:
> refs/heads/master 22bb4923e -> eaf5795e1
>
>
> TS-2766: HdrHeap::coalesce_str_heaps doesn't properly calculate new heap size
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/1630af7f
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/1630af7f
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/1630af7f
>
> Branch: refs/heads/master
> Commit: 1630af7f76643c86751398838fbdf8a1363ce292
> Parents: 0df8e86
> Author: Brian Geffon <br...@apache.org>
> Authored: Thu May 1 13:50:55 2014 -0700
> Committer: Brian Geffon <br...@apache.org>
> Committed: Thu May 1 13:50:55 2014 -0700
>
> ----------------------------------------------------------------------
> proxy/hdrs/HTTP.cc | 11 +++++++++++
> proxy/hdrs/HTTP.h | 1 +
> proxy/hdrs/HdrHeap.cc | 48 +++++++++++++++++++++++++++++++++++++---------
> proxy/hdrs/HdrHeap.h | 6 +++---
> proxy/hdrs/MIME.cc | 21 ++++++++++++++++++++
> proxy/hdrs/MIME.h | 2 ++
> proxy/hdrs/URL.cc | 15 +++++++++++++++
> proxy/hdrs/URL.h | 1 +
> 8 files changed, 93 insertions(+), 12 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/HTTP.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/HTTP.cc b/proxy/hdrs/HTTP.cc
> index 4ce25c8..44cf881 100644
> --- a/proxy/hdrs/HTTP.cc
> +++ b/proxy/hdrs/HTTP.cc
> @@ -1687,6 +1687,17 @@ HTTPHdrImpl::move_strings(HdrStrHeap *new_heap)
> }
> }
>
> +size_t
> +HTTPHdrImpl::strings_length() {
> + size_t ret = 0;
> + if (m_polarity == HTTP_TYPE_REQUEST) {
> + ret += u.req.m_len_method;
> + } else if (m_polarity == HTTP_TYPE_RESPONSE) {
> + ret += u.resp.m_len_reason;
> + }
> + return ret;
> +}
> +
> void
> HTTPHdrImpl::check_strings(HeapCheck *heaps, int num_heaps)
> {
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/HTTP.h
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/HTTP.h b/proxy/hdrs/HTTP.h
> index e6eabac..cddd2fe 100644
> --- a/proxy/hdrs/HTTP.h
> +++ b/proxy/hdrs/HTTP.h
> @@ -267,6 +267,7 @@ struct HTTPHdrImpl:public HdrHeapObjImpl
> int marshal(MarshalXlate *ptr_xlate, int num_ptr, MarshalXlate *str_xlate, int num_str);
> void unmarshal(intptr_t offset);
> void move_strings(HdrStrHeap *new_heap);
> + size_t strings_length();
size_t strings_length() const;
>
> // Sanity Check Functions
> void check_strings(HeapCheck *heaps, int num_heaps);
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/HdrHeap.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/HdrHeap.cc b/proxy/hdrs/HdrHeap.cc
> index a70c0ab..296f766 100644
> --- a/proxy/hdrs/HdrHeap.cc
> +++ b/proxy/hdrs/HdrHeap.cc
> @@ -369,15 +369,7 @@ HdrHeap::coalesce_str_heaps(int incoming_size)
> ink_assert(incoming_size >= 0);
> ink_assert(m_writeable);
>
> - if (m_read_write_heap) {
> - new_heap_size += m_read_write_heap->m_heap_size;
> - }
> -
> - for (int i = 0; i < HDR_BUF_RONLY_HEAPS; i++) {
> - if (m_ronly_heap[i].m_heap_start != NULL) {
> - new_heap_size += m_ronly_heap[i].m_heap_len;
> - }
> - }
> + new_heap_size += required_space_for_evacuation();
>
> HdrStrHeap *new_heap = new_HdrStrHeap(new_heap_size);
> evacuate_from_str_heaps(new_heap);
> @@ -448,6 +440,44 @@ HdrHeap::evacuate_from_str_heaps(HdrStrHeap * new_heap)
> }
> }
>
> +size_t
> +HdrHeap::required_space_for_evacuation()
size_t HdrHeap::required_space_for_evacuation() const
> +{
> + size_t ret = 0;
> + HdrHeap *h = this;
> + while (h) {
> + char *data = h->m_data_start;
> +
> + while (data < h->m_free_start) {
> + HdrHeapObjImpl *obj = (HdrHeapObjImpl *) data;
> +
> + switch (obj->m_type) {
> + case HDR_HEAP_OBJ_URL:
> + ret += ((URLImpl *) obj)->strings_length();
> + break;
> + case HDR_HEAP_OBJ_HTTP_HEADER:
> + ret += ((HTTPHdrImpl *) obj)->strings_length();
> + break;
> + case HDR_HEAP_OBJ_MIME_HEADER:
> + ret += ((MIMEHdrImpl *) obj)->strings_length();
> + break;
> + case HDR_HEAP_OBJ_FIELD_BLOCK:
> + ret += ((MIMEFieldBlockImpl *) obj)->strings_length();
> + break;
> + case HDR_HEAP_OBJ_EMPTY:
> + case HDR_HEAP_OBJ_RAW:
> + // Nothing to do
> + break;
> + default:
> + ink_release_assert(0);
> + }
> + data = data + obj->m_length;
> + }
> + h = h->m_next;
> + }
> + return ret;
> +}
> +
> void
> HdrHeap::sanity_check_strs()
> {
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/HdrHeap.h
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/HdrHeap.h b/proxy/hdrs/HdrHeap.h
> index 787d99a..c584bb5 100644
> --- a/proxy/hdrs/HdrHeap.h
> +++ b/proxy/hdrs/HdrHeap.h
> @@ -51,11 +51,10 @@
> // heaps are hand unrolled in the code. Chaning
> // this value requires a full pass through HdrBuf.cc
> // to fix the unrolled operations
Is the comment true? Does changing this require changing the unrolled operations?
> -#define HDR_BUF_RONLY_HEAPS 3
> +#define HDR_BUF_RONLY_HEAPS 6
>
> -// Changed these so they for sure fit one normal TCP packet full of headers.
> #define HDR_HEAP_DEFAULT_SIZE 2048
> -#define HDR_STR_HEAP_DEFAULT_SIZE 2048
> +#define HDR_STR_HEAP_DEFAULT_SIZE 4096
>
> #define HDR_MAX_ALLOC_SIZE (HDR_HEAP_DEFAULT_SIZE - sizeof(HdrHeap))
> #define HDR_HEAP_HDR_SIZE ROUND(sizeof(HdrHeap), HDR_PTR_SIZE)
> @@ -262,6 +261,7 @@ public:
> int demote_rw_str_heap();
> void coalesce_str_heaps(int incoming_size = 0);
> void evacuate_from_str_heaps(HdrStrHeap * new_heap);
> + size_t required_space_for_evacuation();
> int attach_str_heap(char *h_start, int h_len, RefCountObj * h_ref_obj, int *index);
>
> /** Struct to prevent garbage collection on heaps.
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/MIME.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/MIME.cc b/proxy/hdrs/MIME.cc
> index 084ecef..95e5d09 100644
> --- a/proxy/hdrs/MIME.cc
> +++ b/proxy/hdrs/MIME.cc
> @@ -3594,6 +3594,22 @@ MIMEFieldBlockImpl::move_strings(HdrStrHeap *new_heap)
> }
> }
>
> +size_t
> +MIMEFieldBlockImpl::strings_length()
> +{
> + size_t ret = 0;
> + for (uint32_t index = 0; index < m_freetop; index++) {
> + MIMEField *field = &(m_field_slots[index]);
> +
> + if (field->m_readiness == MIME_FIELD_SLOT_READINESS_LIVE ||
> + field->m_readiness == MIME_FIELD_SLOT_READINESS_DETACHED) {
> + ret += field->m_len_name;
> + ret += field->m_len_value;
> + }
> + }
> + return ret;
> +}
> +
> void
> MIMEFieldBlockImpl::check_strings(HeapCheck *heaps, int num_heaps)
> {
> @@ -3629,6 +3645,11 @@ MIMEHdrImpl::move_strings(HdrStrHeap *new_heap)
> m_first_fblock.move_strings(new_heap);
> }
>
> +size_t
> +MIMEHdrImpl::strings_length() {
> + return m_first_fblock.strings_length();
> +}
> +
> void
> MIMEHdrImpl::check_strings(HeapCheck *heaps, int num_heaps)
> {
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/MIME.h
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/MIME.h b/proxy/hdrs/MIME.h
> index 04bc35b..74a63c3 100644
> --- a/proxy/hdrs/MIME.h
> +++ b/proxy/hdrs/MIME.h
> @@ -170,6 +170,7 @@ struct MIMEFieldBlockImpl:public HdrHeapObjImpl
> int marshal(MarshalXlate * ptr_xlate, int num_ptr, MarshalXlate * str_xlate, int num_str);
> void unmarshal(intptr_t offset);
> void move_strings(HdrStrHeap * new_heap);
> + size_t strings_length();
>
> // Sanity Check Functions
> void check_strings(HeapCheck * heaps, int num_heaps);
> @@ -242,6 +243,7 @@ struct MIMEHdrImpl:public HdrHeapObjImpl
> int marshal(MarshalXlate * ptr_xlate, int num_ptr, MarshalXlate * str_xlate, int num_str);
> void unmarshal(intptr_t offset);
> void move_strings(HdrStrHeap * new_heap);
> + size_t strings_length();
>
> // Sanity Check Functions
> void check_strings(HeapCheck * heaps, int num_heaps);
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/URL.cc
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/URL.cc b/proxy/hdrs/URL.cc
> index 2dea36f..34fec21 100644
> --- a/proxy/hdrs/URL.cc
> +++ b/proxy/hdrs/URL.cc
> @@ -359,6 +359,21 @@ URLImpl::move_strings(HdrStrHeap * new_heap)
> // HDR_MOVE_STR(m_ptr_printed_string, m_len_printed_string);
> }
>
> +size_t
> +URLImpl::strings_length() {
size_t URLImpl::strings_length() const
> + size_t ret = 0;
> + ret += m_len_scheme;
> + ret += m_len_user;
> + ret += m_len_password;
> + ret += m_len_host;
> + ret += m_len_port;
> + ret += m_len_path;
> + ret += m_len_params;
> + ret += m_len_query;
> + ret += m_len_fragment;
> + return ret;
> +}
> +
> void
> URLImpl::check_strings(HeapCheck * heaps, int num_heaps)
> {
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/1630af7f/proxy/hdrs/URL.h
> ----------------------------------------------------------------------
> diff --git a/proxy/hdrs/URL.h b/proxy/hdrs/URL.h
> index c60d1c2..0a6cd81 100644
> --- a/proxy/hdrs/URL.h
> +++ b/proxy/hdrs/URL.h
> @@ -80,6 +80,7 @@ struct URLImpl:public HdrHeapObjImpl
> int marshal(MarshalXlate *str_xlate, int num_xlate);
> void unmarshal(intptr_t offset);
> void move_strings(HdrStrHeap *new_heap);
> + size_t strings_length();
>
> // Sanity Check Functions
> void check_strings(HeapCheck *heaps, int num_heaps);
>