This is an automated email from the ASF dual-hosted git repository.

bcall pushed a commit to branch 8.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/8.1.x by this push:
     new 50a5366  Add MIMEHdr Garbage Collection to HPACK Dynamic Table
50a5366 is described below

commit 50a5366574c2569e24fc80cf4f6e102b8330cc79
Author: Masaori Koshiba <masa...@apache.org>
AuthorDate: Tue Jul 2 13:04:20 2019 +0900

    Add MIMEHdr Garbage Collection to HPACK Dynamic Table
    
    This is a combination of 2 commits.
    
    1. Reverse internal order of HPACK Dynamic Table Entries
    
    Prioir this change, HpackDynamicTable::add_header_field() always inserts
    the entry in front of the vector.
    
    (cherry picked from commit 206384e84b6ae876c41028b44bf4c5cf8585f748)
    
    Conflicts:
        proxy/http2/HPACK.cc
    
    2. Add MIMEHdr Garbage Collection to HPACK Dynamic Table
    
    Prior this change, the size of HdrHeap which is owned by MIMEHdr of 
HpackDynamicTable had no limit.
    Because when MIMEFiled is deleted the allocated memory of the HdrHeap was 
not freed.
    To mitigate this issue, when HdrHeap size exceeds the threshold, 
HpackDynamicTable start using new MIMEHdr and HdrHeap.
    The old MIMEHdr and HdrHeap will be freed, when all MIMEFiled is deleted by 
HPACK Dynamic Table Entry Eviction.
    
    (cherry picked from commit 2bbcc48195e98b65b681a7e28c9ae1c2ab5d6994)
    
    Conflicts:
        proxy/http2/HPACK.cc
---
 proxy/hdrs/HdrHeap.cc |  18 +++++++
 proxy/hdrs/HdrHeap.h  |   2 +
 proxy/http2/HPACK.cc  | 131 +++++++++++++++++++++++++++++++++++++-------------
 proxy/http2/HPACK.h   |  18 +++----
 4 files changed, 125 insertions(+), 44 deletions(-)

diff --git a/proxy/hdrs/HdrHeap.cc b/proxy/hdrs/HdrHeap.cc
index 0de45b2..0a8831f 100644
--- a/proxy/hdrs/HdrHeap.cc
+++ b/proxy/hdrs/HdrHeap.cc
@@ -1117,6 +1117,24 @@ HdrHeap::dump_heap(int len)
   fprintf(stderr, "\n-------------- End header heap dump -----------\n");
 }
 
+uint64_t
+HdrHeap::total_used_size() const
+{
+  uint64_t size    = 0;
+  const HdrHeap *h = this;
+
+  while (h) {
+    size += (h->m_free_start - h->m_data_start);
+    h = h->m_next;
+  }
+
+  return size;
+}
+
+//
+// HdrStrHeap
+//
+
 void
 HdrStrHeap::free()
 {
diff --git a/proxy/hdrs/HdrHeap.h b/proxy/hdrs/HdrHeap.h
index faa8c48..4268768 100644
--- a/proxy/hdrs/HdrHeap.h
+++ b/proxy/hdrs/HdrHeap.h
@@ -266,6 +266,8 @@ public:
   size_t required_space_for_evacuation();
   bool attach_str_heap(char *h_start, int h_len, RefCountObj *h_ref_obj, int 
*index);
 
+  uint64_t total_used_size() const;
+
   /** Struct to prevent garbage collection on heaps.
       This bumps the reference count to the heap containing the pointer
       while the instance of this class exists. When it goes out of scope
diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc
index 1c45d1e..80b4c06 100644
--- a/proxy/http2/HPACK.cc
+++ b/proxy/http2/HPACK.cc
@@ -166,6 +166,21 @@ static const StaticTable STATIC_TABLE[] = {{"", ""},
                                            {"via", ""},
                                            {"www-authenticate", ""}};
 
+/**
+  Threshold for total HdrHeap size which used by HPAK Dynamic Table.
+  The HdrHeap is filled by MIMEHdrImpl and MIMEFieldBlockImpl like below.
+  This threshold allow to allocate 3 HdrHeap at maximum.
+
+                     +------------------+-----------------------------+
+   HdrHeap 1 (2048): | MIMEHdrImpl(592) | MIMEFieldBlockImpl(528) x 2 |
+                     +------------------+-----------------------------+--...--+
+   HdrHeap 2 (4096): | MIMEFieldBlockImpl(528) x 7                            |
+                     
+------------------------------------------------+--...--+--...--+
+   HdrHeap 3 (8192): | MIMEFieldBlockImpl(528) x 15                            
       |
+                     
+------------------------------------------------+--...--+--...--+
+*/
+static constexpr uint32_t HPACK_HDR_HEAP_THRESHOLD = sizeof(MIMEHdrImpl) + 
sizeof(MIMEFieldBlockImpl) * (2 + 7 + 15);
+
 /******************
  * Local functions
  ******************/
@@ -318,10 +333,28 @@ HpackIndexingTable::update_maximum_size(uint32_t new_size)
   return _dynamic_table->update_maximum_size(new_size);
 }
 
+//
+// HpackDynamicTable
+//
+HpackDynamicTable::~HpackDynamicTable()
+{
+  this->_headers.clear();
+
+  this->_mhdr->fields_clear();
+  this->_mhdr->destroy();
+  delete this->_mhdr;
+
+  if (this->_mhdr_old != nullptr) {
+    this->_mhdr_old->fields_clear();
+    this->_mhdr_old->destroy();
+    delete this->_mhdr_old;
+  }
+}
+
 const MIMEField *
 HpackDynamicTable::get_header_field(uint32_t index) const
 {
-  return _headers.at(index);
+  return this->_headers.at(this->_headers.size() - index - 1);
 }
 
 void
@@ -337,28 +370,18 @@ HpackDynamicTable::add_header_field(const MIMEField 
*field)
     // It is not an error to attempt to add an entry that is larger than
     // the maximum size; an attempt to add an entry larger than the entire
     // table causes the table to be emptied of all existing entries.
-    _headers.clear();
-    _mhdr->fields_clear();
-    _current_size = 0;
+    this->_headers.clear();
+    this->_mhdr->fields_clear();
+    this->_current_size = 0;
   } else {
-    _current_size += header_size;
-    while (_current_size > _maximum_size) {
-      int last_name_len, last_value_len;
-      MIMEField *last_field = _headers.back();
-
-      last_field->name_get(&last_name_len);
-      last_field->value_get(&last_value_len);
-      _current_size -= ADDITIONAL_OCTETS + last_name_len + last_value_len;
+    this->_current_size += header_size;
+    this->_evict_overflowed_entries();
 
-      _headers.erase(_headers.begin() + _headers.size() - 1);
-      _mhdr->field_delete(last_field, false);
-    }
-
-    MIMEField *new_field = _mhdr->field_create(name, name_len);
-    new_field->value_set(_mhdr->m_heap, _mhdr->m_mime, value, value_len);
-    _mhdr->field_attach(new_field);
+    MIMEField *new_field = this->_mhdr->field_create(name, name_len);
+    new_field->value_set(this->_mhdr->m_heap, this->_mhdr->m_mime, value, 
value_len);
+    this->_mhdr->field_attach(new_field);
     // XXX Because entire Vec instance is copied, Its too expensive!
-    _headers.insert(_headers.begin(), new_field);
+    this->_headers.push_back(new_field);
   }
 }
 
@@ -384,29 +407,69 @@ HpackDynamicTable::size() const
 bool
 HpackDynamicTable::update_maximum_size(uint32_t new_size)
 {
-  while (_current_size > new_size) {
-    if (_headers.size() <= 0) {
-      return false;
+  this->_maximum_size = new_size;
+  return this->_evict_overflowed_entries();
+}
+
+uint32_t
+HpackDynamicTable::length() const
+{
+  return _headers.size();
+}
+
+bool
+HpackDynamicTable::_evict_overflowed_entries()
+{
+  if (this->_current_size <= this->_maximum_size) {
+    // Do nothing
+    return true;
+  }
+
+  size_t count = 0;
+  for (auto &h : this->_headers) {
+    int name_len, value_len;
+    h->name_get(&name_len);
+    h->value_get(&value_len);
+
+    this->_current_size -= ADDITIONAL_OCTETS + name_len + value_len;
+    this->_mhdr->field_delete(h, false);
+    ++count;
+
+    if (this->_current_size <= this->_maximum_size) {
+      break;
     }
-    int last_name_len, last_value_len;
-    MIMEField *last_field = _headers.back();
+  }
 
-    last_field->name_get(&last_name_len);
-    last_field->value_get(&last_value_len);
-    _current_size -= ADDITIONAL_OCTETS + last_name_len + last_value_len;
+  this->_headers.erase(this->_headers.begin(), this->_headers.begin() + count);
 
-    _headers.erase(_headers.begin() + _headers.size() - 1);
-    _mhdr->field_delete(last_field, false);
+  if (this->_headers.size() == 0) {
+    return false;
   }
 
-  _maximum_size = new_size;
+  this->_mime_hdr_gc();
+
   return true;
 }
 
-uint32_t
-HpackDynamicTable::length() const
+/**
+   When HdrHeap size of current MIMEHdr exceeds the threshold, allocate new 
MIMEHdr and HdrHeap.
+   The old MIMEHdr and HdrHeap will be freed, when all MIMEFiled are deleted 
by HPACK Entry Eviction.
+ */
+void
+HpackDynamicTable::_mime_hdr_gc()
 {
-  return _headers.size();
+  if (this->_mhdr_old == nullptr) {
+    if (this->_mhdr->m_heap->total_used_size() >= HPACK_HDR_HEAP_THRESHOLD) {
+      this->_mhdr_old = this->_mhdr;
+      this->_mhdr     = new MIMEHdr();
+      this->_mhdr->create();
+    }
+  } else {
+    if (this->_mhdr_old->fields_count() == 0) {
+      this->_mhdr_old->destroy();
+      this->_mhdr_old = nullptr;
+    }
+  }
 }
 
 //
diff --git a/proxy/http2/HPACK.h b/proxy/http2/HPACK.h
index 9d0f9f5..5aa8ad5 100644
--- a/proxy/http2/HPACK.h
+++ b/proxy/http2/HPACK.h
@@ -111,13 +111,7 @@ public:
     _mhdr->create();
   }
 
-  ~HpackDynamicTable()
-  {
-    _headers.clear();
-    _mhdr->fields_clear();
-    _mhdr->destroy();
-    delete _mhdr;
-  }
+  ~HpackDynamicTable();
 
   const MIMEField *get_header_field(uint32_t index) const;
   void add_header_field(const MIMEField *field);
@@ -129,10 +123,14 @@ public:
   uint32_t length() const;
 
 private:
-  uint32_t _current_size;
-  uint32_t _maximum_size;
+  bool _evict_overflowed_entries();
+  void _mime_hdr_gc();
+
+  uint32_t _current_size = 0;
+  uint32_t _maximum_size = 0;
 
-  MIMEHdr *_mhdr;
+  MIMEHdr *_mhdr     = nullptr;
+  MIMEHdr *_mhdr_old = nullptr;
   std::vector<MIMEField *> _headers;
 };
 

Reply via email to