You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by br...@apache.org on 2014/05/01 22:51:29 UTC
[1/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't
properly calculate new heap size
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();
// 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 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
-#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 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);
[2/3] git commit: Merge branch 'master' of
https://git-wip-us.apache.org/repos/asf/trafficserver
Posted by br...@apache.org.
Merge branch 'master' of https://git-wip-us.apache.org/repos/asf/trafficserver
Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/7fe85fae
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/7fe85fae
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/7fe85fae
Branch: refs/heads/master
Commit: 7fe85faeecffb61dfcf643ad40df280dcbda4a2b
Parents: 1630af7 22bb492
Author: Brian Geffon <br...@apache.org>
Authored: Thu May 1 13:51:06 2014 -0700
Committer: Brian Geffon <br...@apache.org>
Committed: Thu May 1 13:51:06 2014 -0700
----------------------------------------------------------------------
.gitignore | 3 ++
CHANGES | 2 ++
ci/rat-regex.txt | 5 +++
configure.ac | 1 -
lib/atscppapi/examples/Makefile.am | 41 ++++++++++++-----------
lib/atscppapi/examples/intercept/Makefile.am | 17 +++++-----
lib/ts/test_Vec.cc | 2 +-
mgmt/Main.cc | 2 +-
proxy/Main.cc | 2 +-
proxy/Makefile.am | 1 -
proxy/congest/Makefile.am | 1 -
proxy/http/Makefile.am | 1 -
proxy/logging/Makefile.am | 1 -
proxy/spdy/Makefile.am | 1 -
proxy/spdy/SpdySM.cc | 9 ++++-
rc/trafficserver.service.in | 17 ++++++++++
16 files changed, 68 insertions(+), 38 deletions(-)
----------------------------------------------------------------------
Re: [1/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't
properly calculate new heap size
Posted by James Peach <jp...@apache.org>.
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);
>
Re: [1/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't
properly calculate new heap size
Posted by James Peach <jp...@apache.org>.
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);
>
[3/3] git commit: TS-2766: HdrHeap::coalesce_str_heaps doesn't
properly calculate new heap size
Posted by br...@apache.org.
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/eaf5795e
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/eaf5795e
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/eaf5795e
Branch: refs/heads/master
Commit: eaf5795e149185f4834c2beff63c9738e85e833c
Parents: 7fe85fa
Author: Brian Geffon <br...@apache.org>
Authored: Thu May 1 13:51:22 2014 -0700
Committer: Brian Geffon <br...@apache.org>
Committed: Thu May 1 13:51:22 2014 -0700
----------------------------------------------------------------------
CHANGES | 2 ++
1 file changed, 2 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/trafficserver/blob/eaf5795e/CHANGES
----------------------------------------------------------------------
diff --git a/CHANGES b/CHANGES
index 0223ad8..4aa3e26 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,8 @@
-*- coding: utf-8 -*-
Changes with Apache Traffic Server 5.0.0
+ *) [TS-2766] HdrHeap::coalesce_str_heaps doesn't properly calculate new heap size
+
*) [TS-2767] ATS Memory Leak related to SPDY
*) [TS-2757] Fix logging crashes on reconfiguration.