FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: remove callback-based padding · nodejs/node@41637a5 · GitHub

/ node Public

Commit 41637a5

Browse files
addaleax
authored and
Trott
committed
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: #29144 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 5dee17b commit 41637a5

8 files changed

Lines changed: 25 additions & 176 deletions

File tree

‎doc/api/http2.md‎

Lines changed: 15 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1929,6 +1929,11 @@ error will be thrown.
19291929
<!-- YAML
19301930
added: v8.4.0
19311931
changes:
1932+
- version: REPLACEME
1933+
pr-url: https://github.com/nodejs/node/pull/29144
1934+
description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to
1935+
providing `PADDING_STRATEGY_ALIGNED` and `selectPadding`
1936+
has been removed.
19321937
- version: v12.4.0
19331938
pr-url: https://github.com/nodejs/node/pull/27782
19341939
description: The `options` parameter now supports `net.createServer()`
@@ -1975,9 +1980,6 @@ changes:
19751980
* `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum
19761981
amount of padding, as determined by the internal implementation, is to
19771982
be applied.
1978-
* `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user
1979-
provided `options.selectPadding()` callback is to be used to determine
1980-
the amount of padding.
19811983
* `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply
19821984
enough padding to ensure that the total frame length, including the
19831985
9-byte header, is a multiple of 8. For each frame, however, there is a
@@ -1989,9 +1991,6 @@ changes:
19891991
streams for the remote peer as if a `SETTINGS` frame had been received. Will
19901992
be overridden if the remote peer sets its own value for
19911993
`maxConcurrentStreams`. **Default:** `100`.
1992-
* `selectPadding` {Function} When `options.paddingStrategy` is equal to
1993-
`http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function
1994-
used to determine the padding. See [Using `options.selectPadding()`][].
19951994
* `settings` {HTTP/2 Settings Object} The initial settings to send to the
19961995
remote peer upon connection.
19971996
* `Http1IncomingMessage` {http.IncomingMessage} Specifies the
@@ -2044,6 +2043,11 @@ server.listen(80);
20442043
<!-- YAML
20452044
added: v8.4.0
20462045
changes:
2046+
- version: REPLACEME
2047+
pr-url: https://github.com/nodejs/node/pull/29144
2048+
description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to
2049+
providing `PADDING_STRATEGY_ALIGNED` and `selectPadding`
2050+
has been removed.
20472051
- version: v10.12.0
20482052
pr-url: https://github.com/nodejs/node/pull/22956
20492053
description: Added the `origins` option to automatically send an `ORIGIN`
@@ -2090,9 +2094,6 @@ changes:
20902094
* `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum
20912095
amount of padding, as determined by the internal implementation, is to
20922096
be applied.
2093-
* `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user
2094-
provided `options.selectPadding()` callback is to be used to determine
2095-
the amount of padding.
20962097
* `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply
20972098
enough padding to ensure that the total frame length, including the
20982099
9-byte header, is a multiple of 8. For each frame, however, there is a
@@ -2104,9 +2105,6 @@ changes:
21042105
streams for the remote peer as if a `SETTINGS` frame had been received. Will
21052106
be overridden if the remote peer sets its own value for
21062107
`maxConcurrentStreams`. **Default:** `100`.
2107-
* `selectPadding` {Function} When `options.paddingStrategy` is equal to
2108-
`http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function
2109-
used to determine the padding. See [Using `options.selectPadding()`][].
21102108
* `settings` {HTTP/2 Settings Object} The initial settings to send to the
21112109
remote peer upon connection.
21122110
* ...: Any [`tls.createServer()`][] options can be provided. For
@@ -2146,6 +2144,11 @@ server.listen(80);
21462144
<!-- YAML
21472145
added: v8.4.0
21482146
changes:
2147+
- version: REPLACEME
2148+
pr-url: https://github.com/nodejs/node/pull/29144
2149+
description: The `PADDING_STRATEGY_CALLBACK` has been made equivalent to
2150+
providing `PADDING_STRATEGY_ALIGNED` and `selectPadding`
2151+
has been removed.
21492152
- version: v8.9.3
21502153
pr-url: https://github.com/nodejs/node/pull/17105
21512154
description: Added the `maxOutstandingPings` option with a default limit of
@@ -2191,9 +2194,6 @@ changes:
21912194
* `http2.constants.PADDING_STRATEGY_MAX` - Specifies that the maximum
21922195
amount of padding, as determined by the internal implementation, is to
21932196
be applied.
2194-
* `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user
2195-
provided `options.selectPadding()` callback is to be used to determine
2196-
the amount of padding.
21972197
* `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply
21982198
enough padding to ensure that the total frame length, including the
21992199
9-byte header, is a multiple of 8. For each frame, however, there is a
@@ -2205,9 +2205,6 @@ changes:
22052205
streams for the remote peer as if a `SETTINGS` frame had been received. Will
22062206
be overridden if the remote peer sets its own value for
22072207
`maxConcurrentStreams`. **Default:** `100`.
2208-
* `selectPadding` {Function} When `options.paddingStrategy` is equal to
2209-
`http2.constants.PADDING_STRATEGY_CALLBACK`, provides the callback function
2210-
used to determine the padding. See [Using `options.selectPadding()`][].
22112208
* `settings` {HTTP/2 Settings Object} The initial settings to send to the
22122209
remote peer upon connection.
22132210
* `createConnection` {Function} An optional callback that receives the `URL`
@@ -2389,30 +2386,6 @@ properties.
23892386

23902387
All additional properties on the settings object are ignored.
23912388

2392-
### Using `options.selectPadding()`
2393-
2394-
When `options.paddingStrategy` is equal to
2395-
`http2.constants.PADDING_STRATEGY_CALLBACK`, the HTTP/2 implementation will
2396-
consult the `options.selectPadding()` callback function, if provided, to
2397-
determine the specific amount of padding to use per `HEADERS` and `DATA` frame.
2398-
2399-
The `options.selectPadding()` function receives two numeric arguments,
2400-
`frameLen` and `maxFrameLen` and must return a number `N` such that
2401-
`frameLen <= N <= maxFrameLen`.
2402-
2403-
```js
2404-
const http2 = require('http2');
2405-
const server = http2.createServer({
2406-
paddingStrategy: http2.constants.PADDING_STRATEGY_CALLBACK,
2407-
selectPadding(frameLen, maxFrameLen) {
2408-
return maxFrameLen;
2409-
}
2410-
});
2411-
```
2412-
2413-
The `options.selectPadding()` function is invoked once for *every* `HEADERS` and
2414-
`DATA` frame. This has a definite noticeable impact on performance.
2415-
24162389
### Error Handling
24172390

24182391
There are several types of error conditions that may arise when using the
@@ -3498,7 +3471,6 @@ following additional properties:
34983471
[RFC 8441]: https://tools.ietf.org/html/rfc8441
34993472
[Readable Stream]: stream.html#stream_class_stream_readable
35003473
[Stream]: stream.html#stream_stream
3501-
[Using `options.selectPadding()`]: #http2_using_options_selectpadding
35023474
[`'checkContinue'`]: #http2_event_checkcontinue
35033475
[`'connect'`]: #http2_event_connect
35043476
[`'request'`]: #http2_event_request

‎lib/internal/http2/core.js‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -194,10 +194,6 @@ const kType = Symbol('type');
194194
const kWriteGeneric = Symbol('write-generic');
195195

196196
const {
197-
paddingBuffer,
198-
PADDING_BUF_FRAME_LENGTH,
199-
PADDING_BUF_MAX_PAYLOAD_LENGTH,
200-
PADDING_BUF_RETURN_VALUE,
201197
kBitfield,
202198
kSessionPriorityListenerCount,
203199
kSessionFrameErrorListenerCount,
@@ -617,20 +613,6 @@ function onGoawayData(code, lastStreamID, buf) {
617613
}
618614
}
619615

620-
// Returns the padding to use per frame. The selectPadding callback is set
621-
// on the options. It is invoked with two arguments, the frameLen, and the
622-
// maxPayloadLen. The method must return a numeric value within the range
623-
// frameLen <= n <= maxPayloadLen.
624-
function onSelectPadding() {
625-
const session = this[kOwner];
626-
if (session.destroyed)
627-
return;
628-
const fn = session[kSelectPadding];
629-
const frameLen = paddingBuffer[PADDING_BUF_FRAME_LENGTH];
630-
const maxFramePayloadLen = paddingBuffer[PADDING_BUF_MAX_PAYLOAD_LENGTH];
631-
paddingBuffer[PADDING_BUF_RETURN_VALUE] = fn(frameLen, maxFramePayloadLen);
632-
}
633-
634616
// When a ClientHttp2Session is first created, the socket may not yet be
635617
// connected. If request() is called during this time, the actual request
636618
// will be deferred until the socket is ready to go.
@@ -3018,7 +3000,6 @@ binding.setCallbackFunctions(
30183000
onGoawayData,
30193001
onAltSvc,
30203002
onOrigin,
3021-
onSelectPadding,
30223003
onStreamTrailers,
30233004
onStreamClose
30243005
);

‎src/env.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,6 @@ constexpr size_t kFsStatsBufferLength =
426426
V(http2session_on_origin_function, v8::Function) \
427427
V(http2session_on_ping_function, v8::Function) \
428428
V(http2session_on_priority_function, v8::Function) \
429-
V(http2session_on_select_padding_function, v8::Function) \
430429
V(http2session_on_settings_function, v8::Function) \
431430
V(http2session_on_stream_close_function, v8::Function) \
432431
V(http2session_on_stream_trailers_function, v8::Function) \

‎src/node_http2.cc‎

Lines changed: 3 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -840,32 +840,6 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen,
840840
return maxPayloadLen;
841841
}
842842

843-
// Used as one of the Padding Strategy functions. Uses a callback to JS land
844-
// to determine the amount of padding for the current frame. This option is
845-
// rather more expensive because of the JS boundary cross. It generally should
846-
// not be the preferred option.
847-
ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
848-
size_t maxPayloadLen) {
849-
if (frameLen == 0) return 0;
850-
Debug(this, "using callback to determine padding");
851-
Isolate* isolate = env()->isolate();
852-
HandleScope handle_scope(isolate);
853-
Local<Context> context = env()->context();
854-
Context::Scope context_scope(context);
855-
856-
AliasedUint32Array& buffer = env()->http2_state()->padding_buffer;
857-
buffer[PADDING_BUF_FRAME_LENGTH] = frameLen;
858-
buffer[PADDING_BUF_MAX_PAYLOAD_LENGTH] = maxPayloadLen;
859-
buffer[PADDING_BUF_RETURN_VALUE] = frameLen;
860-
MakeCallback(env()->http2session_on_select_padding_function(), 0, nullptr);
861-
uint32_t retval = buffer[PADDING_BUF_RETURN_VALUE];
862-
retval = std::min(retval, static_cast<uint32_t>(maxPayloadLen));
863-
retval = std::max(retval, static_cast<uint32_t>(frameLen));
864-
Debug(this, "using padding size %d", retval);
865-
return retval;
866-
}
867-
868-
869843
// Write data received from the i/o stream to the underlying nghttp2_session.
870844
// On each call to nghttp2_session_mem_recv, nghttp2 will begin calling the
871845
// various callback functions. Each of these will typically result in a call
@@ -1251,9 +1225,6 @@ ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle,
12511225
case PADDING_STRATEGY_ALIGNED:
12521226
padding = session->OnDWordAlignedPadding(padding, maxPayloadLen);
12531227
break;
1254-
case PADDING_STRATEGY_CALLBACK:
1255-
padding = session->OnCallbackPadding(padding, maxPayloadLen);
1256-
break;
12571228
}
12581229
return padding;
12591230
}
@@ -3024,7 +2995,7 @@ void nghttp2_header::MemoryInfo(MemoryTracker* tracker) const {
30242995

30252996
void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) {
30262997
Environment* env = Environment::GetCurrent(args);
3027-
CHECK_EQ(args.Length(), 12);
2998+
CHECK_EQ(args.Length(), 11);
30282999

30293000
#define SET_FUNCTION(arg, name) \
30303001
CHECK(args[arg]->IsFunction()); \
@@ -3039,9 +3010,8 @@ void SetCallbackFunctions(const FunctionCallbackInfo<Value>& args) {
30393010
SET_FUNCTION(6, goaway_data)
30403011
SET_FUNCTION(7, altsvc)
30413012
SET_FUNCTION(8, origin)
3042-
SET_FUNCTION(9, select_padding)
3043-
SET_FUNCTION(10, stream_trailers)
3044-
SET_FUNCTION(11, stream_close)
3013+
SET_FUNCTION(9, stream_trailers)
3014+
SET_FUNCTION(10, stream_close)
30453015

30463016
#undef SET_FUNCTION
30473017
}
@@ -3062,9 +3032,6 @@ void Initialize(Local<Object> target,
30623032
FIXED_ONE_BYTE_STRING(isolate, (name)), \
30633033
(field)).FromJust()
30643034

3065-
// Initialize the buffer used for padding callbacks
3066-
SET_STATE_TYPEDARRAY(
3067-
"paddingBuffer", state->padding_buffer.GetJSArray());
30683035
// Initialize the buffer used to store the session state
30693036
SET_STATE_TYPEDARRAY(
30703037
"sessionState", state->session_state_buffer.GetJSArray());
@@ -3083,10 +3050,6 @@ void Initialize(Local<Object> target,
30833050

30843051
env->set_http2_state(std::move(state));
30853052

3086-
NODE_DEFINE_CONSTANT(target, PADDING_BUF_FRAME_LENGTH);
3087-
NODE_DEFINE_CONSTANT(target, PADDING_BUF_MAX_PAYLOAD_LENGTH);
3088-
NODE_DEFINE_CONSTANT(target, PADDING_BUF_RETURN_VALUE);
3089-
30903053
NODE_DEFINE_CONSTANT(target, kBitfield);
30913054
NODE_DEFINE_CONSTANT(target, kSessionPriorityListenerCount);
30923055
NODE_DEFINE_CONSTANT(target, kSessionFrameErrorListenerCount);

‎src/node_http2.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -321,11 +321,9 @@ enum padding_strategy_type {
321321
PADDING_STRATEGY_ALIGNED,
322322
// Padding will ensure all data frames are maxFrameSize
323323
PADDING_STRATEGY_MAX,
324-
// Padding will be determined via a JS callback. Note that this can be
325-
// expensive because the callback is called once for every DATA and
326-
// HEADERS frame. For performance reasons, this strategy should be
327-
// avoided.
328-
PADDING_STRATEGY_CALLBACK
324+
// Removed and turned into an alias because it is unreasonably expensive for
325+
// very little benefit.
326+
PADDING_STRATEGY_CALLBACK = PADDING_STRATEGY_ALIGNED
329327
};
330328

331329
enum session_state_flags {
@@ -877,8 +875,6 @@ class Http2Session : public AsyncWrap, public StreamListener {
877875
size_t maxPayloadLen);
878876
ssize_t OnMaxFrameSizePadding(size_t frameLength,
879877
size_t maxPayloadLen);
880-
ssize_t OnCallbackPadding(size_t frameLength,
881-
size_t maxPayloadLen);
882878

883879
// Frame Handler
884880
int HandleDataFrame(const nghttp2_frame* frame);

‎src/node_http2_state.h‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,6 @@ namespace http2 {
5555
IDX_OPTIONS_FLAGS
5656
};
5757

58-
enum Http2PaddingBufferFields {
59-
PADDING_BUF_FRAME_LENGTH,
60-
PADDING_BUF_MAX_PAYLOAD_LENGTH,
61-
PADDING_BUF_RETURN_VALUE,
62-
PADDING_BUF_FIELD_COUNT
63-
};
64-
6558
enum Http2StreamStatisticsIndex {
6659
IDX_STREAM_STATS_ID,
6760
IDX_STREAM_STATS_TIMETOFIRSTBYTE,
@@ -111,11 +104,6 @@ class Http2State {
111104
offsetof(http2_state_internal, session_stats_buffer),
112105
IDX_SESSION_STATS_COUNT,
113106
root_buffer),
114-
padding_buffer(
115-
isolate,
116-
offsetof(http2_state_internal, padding_buffer),
117-
PADDING_BUF_FIELD_COUNT,
118-
root_buffer),
119107
options_buffer(
120108
isolate,
121109
offsetof(http2_state_internal, options_buffer),
@@ -133,7 +121,6 @@ class Http2State {
133121
AliasedFloat64Array stream_state_buffer;
134122
AliasedFloat64Array stream_stats_buffer;
135123
AliasedFloat64Array session_stats_buffer;
136-
AliasedUint32Array padding_buffer;
137124
AliasedUint32Array options_buffer;
138125
AliasedUint32Array settings_buffer;
139126

@@ -144,7 +131,6 @@ class Http2State {
144131
double stream_state_buffer[IDX_STREAM_STATE_COUNT];
145132
double stream_stats_buffer[IDX_STREAM_STATS_COUNT];
146133
double session_stats_buffer[IDX_SESSION_STATS_COUNT];
147-
uint32_t padding_buffer[PADDING_BUF_FIELD_COUNT];
148134
uint32_t options_buffer[IDX_OPTIONS_FLAGS + 1];
149135
uint32_t settings_buffer[IDX_SETTINGS_COUNT + 1];
150136
};

‎test/parallel/test-http2-padding-aligned.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ if (!common.hasCrypto)
55
common.skip('missing crypto');
66
const assert = require('assert');
77
const http2 = require('http2');
8-
const { PADDING_STRATEGY_ALIGNED } = http2.constants;
8+
const { PADDING_STRATEGY_ALIGNED, PADDING_STRATEGY_CALLBACK } = http2.constants;
99
const makeDuplexPair = require('../common/duplexpair');
1010

1111
{
@@ -66,3 +66,6 @@ const makeDuplexPair = require('../common/duplexpair');
6666
}));
6767
req.end();
6868
}
69+
70+
// PADDING_STRATEGY_CALLBACK has been aliased to mean aligned padding.
71+
assert.strictEqual(PADDING_STRATEGY_ALIGNED, PADDING_STRATEGY_CALLBACK);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL