http2: remove callback-based padding

This option is not useful in practice, as mentioned in comments and the
documentation, because the overhead of calling into JS makes it
unreasonably expensive.

PR-URL: https://github.com/nodejs/node/pull/29144
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

4 files changed, 6 insertions, 62 deletions

diff --git a/src/env.h b/src/env.h
index 9a0f98e121..15c2f95313 100644
--- a/src/env.h
+++ b/src/env.h
@@ -426,7 +426,6 @@ constexpr size_t kFsStatsBufferLength =
   V(http2session_on_origin_function, v8::Function)                             \
   V(http2session_on_ping_function, v8::Function)                               \
   V(http2session_on_priority_function, v8::Function)                           \
-  V(http2session_on_select_padding_function, v8::Function)                     \
   V(http2session_on_settings_function, v8::Function)                           \
   V(http2session_on_stream_close_function, v8::Function)                       \
   V(http2session_on_stream_trailers_function, v8::Function)                    \
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 38686dc750..485d54880b 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -840,32 +840,6 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen,
   return maxPayloadLen;
 }
 
-// Used as one of the Padding Strategy functions. Uses a callback to JS land
-// to determine the amount of padding for the current frame. This option is
-// rather more expensive because of the JS boundary cross. It generally should
-// not be the preferred option.
-ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
-                                        size_t maxPayloadLen) {
-  if (frameLen == 0) return 0;
-  Debug(this, "using callback to determine padding");
-  Isolate* isolate = env()->isolate();
-  HandleScope handle_scope(isolate);
-  Local<Context> context = env()->context();
-  Context::Scope context_scope(context);
-
-  AliasedUint32Array& buffer = env()->http2_state()->padding_buffer;
-  buffer[PADDING_BUF_FRAME_LENGTH] = frameLen;
-  buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen;
-  buffer[PADDING_BUF_RETURN_VALUE] = frameLen;
-  MakeCallback(env()->http2session_on_select_padding_function(), 0, nullptr);
-  uint32_t retval = buffer[PADDING_BUF_RETURN_VALUE];
-  retval = std::min(retval, static_cast<uint32_t>(maxPayloadLen));
-  retval = std::max(retval, static_cast<uint32_t>(frameLen));
-  Debug(this, "using padding size %d", retval);
-  return retval;
-}
-
-
 // Write data received from the i/o stream to the underlying nghttp2_session.
 // On each call to nghttp2_session_mem_recv, nghttp2 will begin calling the
 // various callback functions. Each of these will typically result in a call
@@ -1251,9 +1225,6 @@ ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle,
     case PADDING_STRATEGY_ALIGNED:
       padding = session->OnDWordAlignedPadding(padding, maxPayloadLen);
       break;
-    case PADDING_STRATEGY_CALLBACK:
-      padding = session->OnCallbackPadding(padding, maxPayloadLen);
-      break;
   }
   return padding;
 }
@@ -3024,7 +2995,7 @@ void nghttp2_header::MemoryInfo(MemoryTracker* tracker) const {
 
 void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK_EQ(args.Length(), 12);
+  CHECK_EQ(args.Length(), 11);
 
 #define SET_FUNCTION(arg, name)                                               \
   CHECK(args[arg]->IsFunction());                                             \
@@ -3039,9 +3010,8 @@ void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) {
   SET_FUNCTION(6, goaway_data)
   SET_FUNCTION(7, altsvc)
   SET_FUNCTION(8, origin)
-  SET_FUNCTION(9, select_padding)
-  SET_FUNCTION(10, stream_trailers)
-  SET_FUNCTION(11, stream_close)
+  SET_FUNCTION(9, stream_trailers)
+  SET_FUNCTION(10, stream_close)
 
 #undef SET_FUNCTION
 }
@@ -3062,9 +3032,6 @@ void Initialize(Local<Object> target,
               FIXED_ONE_BYTE_STRING(isolate, (name)), \
               (field)).FromJust()
 
-  // Initialize the buffer used for padding callbacks
-  SET_STATE_TYPEDARRAY(
-    "paddingBuffer", state->padding_buffer.GetJSArray());
   // Initialize the buffer used to store the session state
   SET_STATE_TYPEDARRAY(
     "sessionState", state->session_state_buffer.GetJSArray());
@@ -3083,10 +3050,6 @@ void Initialize(Local<Object> target,
 
   env->set_http2_state(std::move(state));
 
-  NODE_DEFINE_CONSTANT(target, PADDING_BUF_FRAME_LENGTH);
-  NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH);
-  NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE);
-
   NODE_DEFINE_CONSTANT(target, kBitfield);
   NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount);
   NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount);
diff --git a/src/node_http2.h b/src/node_http2.h
index 792078d126..db85dc6e5a 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -321,11 +321,9 @@ enum padding_strategy_type {
   PADDING_STRATEGY_ALIGNED,
   // Padding will ensure all data frames are maxFrameSize
   PADDING_STRATEGY_MAX,
-  // Padding will be determined via a JS callback. Note that this can be
-  // expensive because the callback is called once for every DATA and
-  // HEADERS frame. For performance reasons, this strategy should be
-  // avoided.
-  PADDING_STRATEGY_CALLBACK
+  // Removed and turned into an alias because it is unreasonably expensive for
+  // very little benefit.
+  PADDING_STRATEGY_CALLBACK = PADDING_STRATEGY_ALIGNED
 };
 
 enum session_state_flags {
@@ -877,8 +875,6 @@ class Http2Session : public AsyncWrap, public StreamListener {
                                 size_t maxPayloadLen);
   ssize_t OnMaxFrameSizePadding(size_t frameLength,
                                 size_t maxPayloadLen);
-  ssize_t OnCallbackPadding(size_t frameLength,
-                            size_t maxPayloadLen);
 
   // Frame Handler
   int HandleDataFrame(const nghttp2_frame* frame);
diff --git a/src/node_http2_state.h b/src/node_http2_state.h
index 692299a187..1ba3677f47 100644
--- a/src/node_http2_state.h
+++ b/src/node_http2_state.h
@@ -55,13 +55,6 @@ namespace http2 {
     IDX_OPTIONS_FLAGS
   };
 
-  enum Http2PaddingBufferFields {
-    PADDING_BUF_FRAME_LENGTH,
-    PADDING_BUF_MAX_PAYLOAD_LENGTH,
-    PADDING_BUF_RETURN_VALUE,
-    PADDING_BUF_FIELD_COUNT
-  };
-
   enum Http2StreamStatisticsIndex {
     IDX_STREAM_STATS_ID,
     IDX_STREAM_STATS_TIMETOFIRSTBYTE,
@@ -111,11 +104,6 @@ class Http2State {
       offsetof(http2_state_internal, session_stats_buffer),
       IDX_SESSION_STATS_COUNT,
       root_buffer),
-    padding_buffer(
-      isolate,
-      offsetof(http2_state_internal, padding_buffer),
-      PADDING_BUF_FIELD_COUNT,
-      root_buffer),
     options_buffer(
       isolate,
       offsetof(http2_state_internal, options_buffer),
@@ -133,7 +121,6 @@ class Http2State {
   AliasedFloat64Array stream_state_buffer;
   AliasedFloat64Array stream_stats_buffer;
   AliasedFloat64Array session_stats_buffer;
-  AliasedUint32Array padding_buffer;
   AliasedUint32Array options_buffer;
   AliasedUint32Array settings_buffer;
 
@@ -144,7 +131,6 @@ class Http2State {
     double stream_state_buffer[IDX_STREAM_STATE_COUNT];
     double stream_stats_buffer[IDX_STREAM_STATS_COUNT];
     double session_stats_buffer[IDX_SESSION_STATS_COUNT];
-    uint32_t padding_buffer[PADDING_BUF_FIELD_COUNT];
     uint32_t options_buffer[IDX_OPTIONS_FLAGS + 1];
     uint32_t settings_buffer[IDX_SETTINGS_COUNT + 1];
   };