[trafficserver] branch quic-latest updated: fix #2793 rework quic ack creator

[ma...@apache.org]

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

maskit pushed a commit to branch quic-latest
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/quic-latest by this push:
     new 9576d28  fix #2793 rework quic ack creator
9576d28 is described below

commit 9576d280286361a61832eb0e9ff1a2e86947aebf
Author: scw00 <sc...@apache.org>
AuthorDate: Fri Nov 17 10:59:46 2017 +0800

    fix #2793 rework quic ack creator
---
 iocore/net/quic/QUICAckFrameCreator.cc           | 110 ++++++++++++++++++-----
 iocore/net/quic/QUICAckFrameCreator.h            |  28 ++++--
 iocore/net/quic/test/test_QUICAckFrameCreator.cc |  74 +++++++++++++++
 3 files changed, 183 insertions(+), 29 deletions(-)

diff --git a/iocore/net/quic/QUICAckFrameCreator.cc b/iocore/net/quic/QUICAckFrameCreator.cc
index f352d83..89b9525 100644
--- a/iocore/net/quic/QUICAckFrameCreator.cc
+++ b/iocore/net/quic/QUICAckFrameCreator.cc
@@ -28,14 +28,11 @@
 int
 QUICAckFrameCreator::update(QUICPacketNumber packet_number, bool acknowledgable)
 {
-  if (this->_packet_count == MAXIMUM_PACKET_COUNT) {
+  if (this->_packet_numbers.size() == MAXIMUM_PACKET_COUNT) {
     return -1;
   }
-  if (packet_number > this->_largest_ack_number) {
-    this->_largest_ack_number        = packet_number;
-    this->_largest_ack_received_time = Thread::get_hrtime();
-  }
-  this->_packet_numbers[this->_packet_count++] = packet_number - this->_last_ack_number;
+
+  this->_packet_numbers.push_back(packet_number);
   if (acknowledgable && !this->_can_send) {
     this->_can_send = true;
   }
@@ -48,10 +45,10 @@ QUICAckFrameCreator::create()
 {
   std::unique_ptr<QUICAckFrame, QUICFrameDeleterFunc> ack_frame = {nullptr, QUICFrameDeleter::delete_null_frame};
   if (this->_can_send) {
-    ack_frame              = this->_create_ack_frame();
-    this->_last_ack_number = this->_largest_ack_number;
-    this->_can_send        = false;
-    this->_packet_count    = 0;
+    ack_frame           = this->_create_ack_frame();
+    this->_can_send     = false;
+    this->_packet_count = 0;
+    this->_packet_numbers.clear();
   }
   return ack_frame;
 }
@@ -67,37 +64,102 @@ void
 QUICAckFrameCreator::_sort_packet_numbers()
 {
   // TODO Find more smart way
-  std::sort(this->_packet_numbers, this->_packet_numbers + this->_packet_count);
 }
 
 std::unique_ptr<QUICAckFrame, QUICFrameDeleterFunc>
 QUICAckFrameCreator::_create_ack_frame()
 {
   std::unique_ptr<QUICAckFrame, QUICFrameDeleterFunc> ack_frame = {nullptr, QUICFrameDeleter::delete_null_frame};
-  this->_sort_packet_numbers();
-  uint16_t start = this->_packet_numbers[0];
-  uint8_t gap    = 0;
-  int i;
+  this->_packet_numbers.sort();
+  QUICPacketNumber largest_ack_number = this->_packet_numbers.largest_ack_number();
+  QUICPacketNumber last_ack_number    = largest_ack_number;
+
+  size_t i        = 0;
+  uint8_t gap     = 0;
   uint64_t length = 0;
-  for (i = 0, length = 0; i < this->_packet_count; ++i, ++length) {
-    if (this->_packet_numbers[i] == start + length) {
+
+  while (i < this->_packet_numbers.size()) {
+    if (this->_packet_numbers[i] == last_ack_number) {
+      last_ack_number--;
+      length++;
+      i++;
       continue;
     }
+
     if (ack_frame) {
       ack_frame->ack_block_section()->add_ack_block({gap, length});
     } else {
-      uint16_t delay = (Thread::get_hrtime() - this->_largest_ack_received_time) / 1000; // TODO Milliseconds?
-      ack_frame      = QUICFrameFactory::create_ack_frame(this->_largest_ack_number, delay, length);
+      uint16_t delay = (Thread::get_hrtime() - this->_packet_numbers.largest_ack_received_time()) / 1000; // TODO Milliseconds?
+      ack_frame      = QUICFrameFactory::create_ack_frame(largest_ack_number, delay, length);
     }
-    gap    = this->_packet_numbers[i] - this->_packet_numbers[i - 1] - 1;
-    start  = this->_packet_numbers[i];
-    length = 0;
+
+    gap             = last_ack_number - this->_packet_numbers[i];
+    last_ack_number = this->_packet_numbers[i];
+    length          = 0;
   }
+
   if (ack_frame) {
     ack_frame->ack_block_section()->add_ack_block({gap, length});
   } else {
-    uint16_t delay = (Thread::get_hrtime() - this->_largest_ack_received_time) / 1000; // TODO Milliseconds?
-    ack_frame      = QUICFrameFactory::create_ack_frame(this->_largest_ack_number, delay, length);
+    uint16_t delay = (Thread::get_hrtime() - this->_packet_numbers.largest_ack_received_time()) / 1000; // TODO Milliseconds?
+    ack_frame      = QUICFrameFactory::create_ack_frame(largest_ack_number, delay, length);
   }
   return ack_frame;
 }
+
+void
+QUICAckPacketNumbers::push_back(QUICPacketNumber packet_number)
+{
+  if (packet_number > this->_largest_ack_number) {
+    this->_largest_ack_received_time = Thread::get_hrtime();
+    this->_largest_ack_number        = packet_number;
+  }
+
+  this->_packet_numbers.push_back(packet_number);
+}
+
+QUICPacketNumber
+QUICAckPacketNumbers::front()
+{
+  return this->_packet_numbers.front();
+}
+
+QUICPacketNumber
+QUICAckPacketNumbers::back()
+{
+  return this->_packet_numbers.back();
+}
+
+size_t
+QUICAckPacketNumbers::size()
+{
+  return this->_packet_numbers.size();
+}
+
+void
+QUICAckPacketNumbers::clear()
+{
+  this->_packet_numbers.clear();
+  this->_largest_ack_number        = 0;
+  this->_largest_ack_received_time = 0;
+}
+
+QUICPacketNumber
+QUICAckPacketNumbers::largest_ack_number()
+{
+  return this->_largest_ack_number;
+}
+
+ink_hrtime
+QUICAckPacketNumbers::largest_ack_received_time()
+{
+  return this->_largest_ack_received_time;
+}
+
+void
+QUICAckPacketNumbers::sort()
+{
+  //  TODO Find more smart way
+  std::sort(this->_packet_numbers.begin(), this->_packet_numbers.end(),
+            [](QUICPacketNumber a, QUICPacketNumber b) -> bool { return b < a; });
+}
diff --git a/iocore/net/quic/QUICAckFrameCreator.h b/iocore/net/quic/QUICAckFrameCreator.h
index 7251747..da59c5e 100644
--- a/iocore/net/quic/QUICAckFrameCreator.h
+++ b/iocore/net/quic/QUICAckFrameCreator.h
@@ -27,6 +27,28 @@
 #include "QUICTypes.h"
 #include "QUICFrame.h"
 
+class QUICAckPacketNumbers
+{
+public:
+  void push_back(QUICPacketNumber packet_number);
+  QUICPacketNumber front();
+  QUICPacketNumber back();
+  size_t size();
+  void clear();
+  void sort();
+
+  QUICPacketNumber largest_ack_number();
+  ink_hrtime largest_ack_received_time();
+
+  const QUICPacketNumber &operator[](int i) const { return this->_packet_numbers[i]; }
+
+private:
+  QUICPacketNumber _largest_ack_number  = 0;
+  ink_hrtime _largest_ack_received_time = 0;
+
+  std::vector<QUICPacketNumber> _packet_numbers;
+};
+
 class QUICAckFrameCreator
 {
 public:
@@ -57,11 +79,7 @@ public:
 private:
   bool _can_send = false;
 
-  QUICPacketNumber _largest_ack_number  = 0;
-  QUICPacketNumber _last_ack_number     = 0;
-  ink_hrtime _largest_ack_received_time = 0;
-
-  uint16_t _packet_numbers[MAXIMUM_PACKET_COUNT];
+  QUICAckPacketNumbers _packet_numbers;
   uint16_t _packet_count = 0;
 
   void _sort_packet_numbers();
diff --git a/iocore/net/quic/test/test_QUICAckFrameCreator.cc b/iocore/net/quic/test/test_QUICAckFrameCreator.cc
index a519687..1ca0691 100644
--- a/iocore/net/quic/test/test_QUICAckFrameCreator.cc
+++ b/iocore/net/quic/test/test_QUICAckFrameCreator.cc
@@ -64,6 +64,80 @@ TEST_CASE("QUICAckFrameCreator", "[quic]")
   CHECK(frame != nullptr);
   CHECK(frame->num_blocks() == 1);
   CHECK(frame->largest_acknowledged() == 10);
+  CHECK(frame->ack_block_section()->first_ack_block_length() == 1);
+  CHECK(frame->ack_block_section()->begin()->gap() == 2);
+}
+
+TEST_CASE("QUICAckFrameCreator_loss_recover", "[quic]")
+{
+  QUICAckFrameCreator creator;
+  std::unique_ptr<QUICAckFrame, QUICFrameDeleterFunc> frame = {nullptr, nullptr};
+
+  // Initial state
+  frame = creator.create();
+  CHECK(frame == nullptr);
+
+  creator.update(2, true);
+  creator.update(5, true);
+  creator.update(6, true);
+  creator.update(8, true);
+  creator.update(9, true);
+
+  frame = creator.create();
+  CHECK(frame != nullptr);
+  CHECK(frame->num_blocks() == 2);
+  CHECK(frame->largest_acknowledged() == 9);
   CHECK(frame->ack_block_section()->first_ack_block_length() == 2);
+  CHECK(frame->ack_block_section()->begin()->gap() == 1);
+
+  frame = creator.create();
+  CHECK(frame == nullptr);
+
+  creator.update(7, true);
+  creator.update(4, true);
+  frame = creator.create();
+  CHECK(frame != nullptr);
+  CHECK(frame->num_blocks() == 1);
+  CHECK(frame->largest_acknowledged() == 7);
+  CHECK(frame->ack_block_section()->first_ack_block_length() == 1);
   CHECK(frame->ack_block_section()->begin()->gap() == 2);
 }
+
+TEST_CASE("QUICAckFrameCreator_QUICAckPacketNumbers", "[quic]")
+{
+  QUICAckPacketNumbers packet_numbers;
+
+  CHECK(packet_numbers.size() == 0);
+  CHECK(packet_numbers.largest_ack_number() == 0);
+  CHECK(packet_numbers.largest_ack_received_time() == 0);
+
+  Thread::get_hrtime_updated();
+
+  packet_numbers.push_back(3);
+  CHECK(packet_numbers.size() == 1);
+  CHECK(packet_numbers.largest_ack_number() == 3);
+
+  ink_hrtime ti = packet_numbers.largest_ack_received_time();
+  CHECK(packet_numbers.largest_ack_received_time() != 0);
+
+  Thread::get_hrtime_updated();
+
+  packet_numbers.push_back(2);
+  CHECK(packet_numbers.size() == 2);
+  CHECK(packet_numbers.largest_ack_number() == 3);
+  CHECK(packet_numbers.largest_ack_received_time() == ti);
+
+  Thread::get_hrtime_updated();
+
+  packet_numbers.push_back(10);
+  CHECK(packet_numbers.size() == 3);
+  CHECK(packet_numbers.largest_ack_number() == 10);
+  CHECK(packet_numbers.largest_ack_received_time() > ti);
+
+  Thread::get_hrtime_updated();
+
+  packet_numbers.clear();
+  CHECK(packet_numbers.size() == 0);
+  CHECK(packet_numbers.largest_ack_number() == 0);
+  CHECK(packet_numbers.largest_ack_received_time() == 0);
+}

-- 
To stop receiving notification emails like this one, please contact
['"commits@trafficserver.apache.org" <co...@trafficserver.apache.org>'].