Improve weight handling a bit

lib/nghttp2_outbound_item.h

@@ -33,12 +33,12 @@
 #include "nghttp2_frame.h"
 #include "nghttp2_mem.h"
 
-/* A bit higher weight for non-DATA frames */
-#define NGHTTP2_OB_EX_WEIGHT 300
-/* Higher weight for SETTINGS */
-#define NGHTTP2_OB_SETTINGS_WEIGHT 301
-/* Highest weight for PING */
-#define NGHTTP2_OB_PING_WEIGHT 302
+/* A bit higher priority for non-DATA frames */
+#define NGHTTP2_OB_EX_CYCLE 2
+/* Even more higher priority for SETTINGS frame */
+#define NGHTTP2_OB_SETTINGS_CYCLE 1
+/* Highest priority for PING frame */
+#define NGHTTP2_OB_PING_CYCLE 0
 
 /* struct used for HEADERS and PUSH_PROMISE frame */
 typedef struct {
@@ -108,12 +108,14 @@ typedef struct {
   nghttp2_frame frame;
   nghttp2_aux_data aux_data;
   int64_t seq;
-  /* Reset count of weight. See comment for last_cycle in
-     nghttp2_session.h */
+  /* The priority used in priority comparion.  Smaller is served
+     ealier.  For PING, SETTINGS and non-DATA frames (excluding
+     response HEADERS frame) have dedicated cycle value defined above.
+     For DATA frame, cycle is computed by taking into account of
+     effective weight and frame payload length previously sent, so
+     that the amount of transmission is distributed across streams
+     proportional to effective weight (inside a tree). */
   uint64_t cycle;
-  /* The priority used in priority comparion.  Larger is served
-     ealier. */
-  int32_t weight;
   /* nonzero if this object is queued. */
   uint8_t queued;
 } nghttp2_outbound_item;

lib/nghttp2_session.c

@@ -228,14 +228,9 @@ static int outbound_item_compar(const void *lhsx, const void *rhsx) {
   rhs = (const nghttp2_outbound_item *)rhsx;
 
   if (lhs->cycle == rhs->cycle) {
-    if (lhs->weight == rhs->weight) {
     return (lhs->seq < rhs->seq) ? -1 : ((lhs->seq > rhs->seq) ? 1 : 0);
   }
 
-    /* Larger weight has higher precedence */
-    return rhs->weight - lhs->weight;
-  }
-
   return (lhs->cycle < rhs->cycle) ? -1 : 1;
 }
 
@@ -367,7 +362,9 @@ static int session_new(nghttp2_session **session_ptr,
   nghttp2_stream_roots_init(&(*session_ptr)->roots);
 
   (*session_ptr)->next_seq = 0;
-  (*session_ptr)->last_cycle = 1;
+  /* Do +1 so that any HEADERS/DATA frames are scheduled after urgent
+     frames. */
+  (*session_ptr)->last_cycle = NGHTTP2_OB_EX_CYCLE + 1;
 
   (*session_ptr)->remote_window_size = NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE;
   (*session_ptr)->recv_window_size = 0;
@@ -694,9 +691,7 @@ nghttp2_session_reprioritize_stream(nghttp2_session *session,
 void nghttp2_session_outbound_item_init(nghttp2_session *session,
                                         nghttp2_outbound_item *item) {
   item->seq = session->next_seq++;
-  /* We use cycle for DATA only */
-  item->cycle = 0;
-  item->weight = NGHTTP2_OB_EX_WEIGHT;
+  item->cycle = NGHTTP2_OB_EX_CYCLE;
   item->queued = 0;
 
   memset(&item->aux_data, 0, sizeof(nghttp2_aux_data));
@@ -723,12 +718,12 @@ int nghttp2_session_add_item(nghttp2_session *session,
 
       break;
     case NGHTTP2_SETTINGS:
-      item->weight = NGHTTP2_OB_SETTINGS_WEIGHT;
+      item->cycle = NGHTTP2_OB_SETTINGS_CYCLE;
 
       break;
     case NGHTTP2_PING:
       /* Ping has highest priority. */
-      item->weight = NGHTTP2_OB_PING_WEIGHT;
+      item->cycle = NGHTTP2_OB_PING_CYCLE;
 
       break;
     default:
@@ -752,7 +747,6 @@ int nghttp2_session_add_item(nghttp2_session *session,
         item->queued = 1;
       } else if (stream && (stream->state == NGHTTP2_STREAM_RESERVED ||
                             item->aux_data.headers.attach_stream)) {
-        item->weight = stream->effective_weight;
         item->cycle = session->last_cycle;
 
         rv = nghttp2_stream_attach_item(stream, item, session);
@@ -790,7 +784,6 @@ int nghttp2_session_add_item(nghttp2_session *session,
     return NGHTTP2_ERR_DATA_EXIST;
   }
 
-  item->weight = stream->effective_weight;
   item->cycle = session->last_cycle;
 
   rv = nghttp2_stream_attach_item(stream, item, session);
@@ -1986,7 +1979,8 @@ static int session_prep_frame(nghttp2_session *session,
     }
 
     rv = nghttp2_session_pack_data(session, &session->aob.framebufs,
-                                   next_readmax, frame, &item->aux_data.data);
+                                   next_readmax, frame, &item->aux_data.data,
+                                   stream);
     if (rv == NGHTTP2_ERR_DEFERRED) {
       rv = nghttp2_stream_defer_item(stream, NGHTTP2_STREAM_FLAG_DEFERRED_USER,
                                      session);
@@ -2069,8 +2063,7 @@ nghttp2_session_get_next_ob_item(nghttp2_session *session) {
   headers_item = nghttp2_pq_top(&session->ob_ss_pq);
 
   if (session_is_outgoing_concurrent_streams_max(session) ||
-      item->weight > headers_item->weight ||
-      (item->weight == headers_item->weight && item->seq < headers_item->seq)) {
+      outbound_item_compar(item, headers_item) < 0) {
     return item;
   }
 
@@ -2133,8 +2126,7 @@ nghttp2_session_pop_next_ob_item(nghttp2_session *session) {
   headers_item = nghttp2_pq_top(&session->ob_ss_pq);
 
   if (session_is_outgoing_concurrent_streams_max(session) ||
-      item->weight > headers_item->weight ||
-      (item->weight == headers_item->weight && item->seq < headers_item->seq)) {
+      outbound_item_compar(item, headers_item) < 0) {
     nghttp2_pq_pop(&session->ob_pq);
 
     item->queued = 0;
@@ -2249,21 +2241,13 @@ static int session_close_stream_on_goaway(nghttp2_session *session,
   return 0;
 }
 
-static void session_outbound_item_cycle_weight(nghttp2_session *session,
+static void session_outbound_item_schedule(nghttp2_session *session,
                                            nghttp2_outbound_item *item,
-                                               int32_t ini_weight) {
-  if (item->weight == NGHTTP2_MIN_WEIGHT || item->weight > ini_weight) {
-
-    item->weight = ini_weight;
+                                           int32_t weight) {
+  size_t delta = item->frame.hd.length * NGHTTP2_MAX_WEIGHT / weight;
 
-    if (item->cycle == session->last_cycle) {
-      item->cycle = ++session->last_cycle;
-    } else {
-      item->cycle = session->last_cycle;
-    }
-  } else {
-    --item->weight;
-  }
+  session->last_cycle = nghttp2_max(session->last_cycle, item->cycle);
+  item->cycle = session->last_cycle + delta;
 }
 
 /*
@@ -2583,15 +2567,6 @@ static int session_after_frame_sent2(nghttp2_session *session) {
     assert(stream);
     next_item = nghttp2_session_get_next_ob_item(session);
 
-    /* Imagine we hit connection window size limit while sending DATA
-       frame.  If we decrement weight here, its stream might get
-       inferior share because the other streams' weight is not
-       decremented because of flow control. */
-    if (session->remote_window_size > 0 || stream->remote_window_size <= 0) {
-      session_outbound_item_cycle_weight(session, aob->item,
-                                         stream->effective_weight);
-    }
-
     /* If priority of this stream is higher or equal to other stream
        waiting at the top of the queue, we continue to send this
        data. */
@@ -2634,7 +2609,7 @@ static int session_after_frame_sent2(nghttp2_session *session) {
       nghttp2_bufs_reset(framebufs);
 
       rv = nghttp2_session_pack_data(session, framebufs, next_readmax, frame,
-                                     aux_data);
+                                     aux_data, stream);
       if (nghttp2_is_fatal(rv)) {
         return rv;
       }
@@ -6229,7 +6204,8 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags,
 
 int nghttp2_session_pack_data(nghttp2_session *session, nghttp2_bufs *bufs,
                               size_t datamax, nghttp2_frame *frame,
-                              nghttp2_data_aux_data *aux_data) {
+                              nghttp2_data_aux_data *aux_data,
+                              nghttp2_stream *stream) {
   int rv;
   uint32_t data_flags;
   ssize_t payloadlen;
@@ -6242,12 +6218,6 @@ int nghttp2_session_pack_data(nghttp2_session *session, nghttp2_bufs *bufs,
   buf = &bufs->cur->buf;
 
   if (session->callbacks.read_length_callback) {
-    nghttp2_stream *stream;
-
-    stream = nghttp2_session_get_stream(session, frame->hd.stream_id);
-    if (!stream) {
-      return NGHTTP2_ERR_INVALID_ARGUMENT;
-    }
 
     payloadlen = session->callbacks.read_length_callback(
         session, frame->hd.type, stream->stream_id, session->remote_window_size,
@@ -6361,6 +6331,9 @@ int nghttp2_session_pack_data(nghttp2_session *session, nghttp2_bufs *bufs,
     return rv;
   }
 
+  session_outbound_item_schedule(session, stream->item,
+                                 stream->effective_weight);
+
   return 0;
 }
 

lib/nghttp2_session.h

@@ -704,7 +704,8 @@ nghttp2_stream *nghttp2_session_get_stream_raw(nghttp2_session *session,
  */
 int nghttp2_session_pack_data(nghttp2_session *session, nghttp2_bufs *bufs,
                               size_t datamax, nghttp2_frame *frame,
-                              nghttp2_data_aux_data *aux_data);
+                              nghttp2_data_aux_data *aux_data,
+                              nghttp2_stream *stream);
 
 /*
  * Returns top of outbound frame queue. This function returns NULL if

lib/nghttp2_stream.c

@@ -102,17 +102,15 @@ static int stream_push_item(nghttp2_stream *stream, nghttp2_session *session) {
     return 0;
   }
 
-  if (item->weight > stream->effective_weight) {
-    item->weight = stream->effective_weight;
-  }
-
-  item->cycle = session->last_cycle;
-
   switch (item->frame.hd.type) {
   case NGHTTP2_DATA:
+    /* Penalize low weight stream */
+    item->cycle =
+        session->last_cycle + NGHTTP2_MAX_WEIGHT / stream->effective_weight;
     rv = nghttp2_pq_push(&session->ob_da_pq, item);
     break;
   case NGHTTP2_HEADERS:
+    item->cycle = session->last_cycle;
     if (stream->state == NGHTTP2_STREAM_RESERVED) {
       rv = nghttp2_pq_push(&session->ob_ss_pq, item);
     } else {

src/nghttp.cc

@@ -2062,11 +2062,12 @@ int communicate(
       dep_stream_id = ANCHOR_ID_HIGH;
     }
 
-    nghttp2_priority_spec_init(&pri_spec, dep_stream_id, config.weight, 0);
-
     for (auto req : requests) {
       for (int i = 0; i < config.multiply; ++i) {
         auto dep = std::make_shared<Dependency>();
+        nghttp2_priority_spec_init(&pri_spec, dep_stream_id,
+                                   config.weight * (i + 1), 0);
+
         client.add_request(std::get<0>(req), std::get<1>(req), std::get<2>(req),
                            pri_spec, std::move(dep));
       }