trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: [1/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't properly calculate new heap size
Date Mon, 12 May 2014 16:17:13 GMT
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 <briang@apache.org>
> Authored: Thu May 1 13:50:55 2014 -0700
> Committer: Brian Geffon <briang@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);
> 


Mime
View raw message