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, bri...@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 <bri...@apache.org> > Authored: Thu May 1 13:50:55 2014 -0700 > Committer: Brian Geffon <bri...@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); >
