From 084eafe50883bc69ae2700023f6c74db03185ba4 Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Thu, 25 Apr 2024 14:52:24 -0400 Subject: [PATCH] perf(ext/http): recover memory for serve and optimize AbortController (#23559) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Max rps without a signal is unchanged, however we can drastically reduce memory usage by not creating the signal until needed, and we can optimize the rps in the case where the signal is created. With a quick memory benchmark, it looks like this helps pretty drastically with # of GCs when benchmarking w/wrk: - 1.42.4: 1763 - canary: 1093 - this patch: 874 This branch: ``` Running 10s test @ http://localhost:8080/ 2 threads and 10 connections Thread Stats Avg Stdev Max +/- Stdev Latency 87.33us 439.95us 20.68ms 99.67% Req/Sec 66.70k 6.39k 74.11k 83.66% 1340255 requests in 10.10s, 191.73MB read Requests/sec: 132696.90 Transfer/sec: 18.98MB cpu: Apple M2 Pro runtime: deno 1.43.0 (aarch64-apple-darwin) file:///Users/matt/Documents/scripts/bench_request.js benchmark time (avg) iter/s (min … max) p75 p99 p995 ----------------------------------------------------------------------------------------------- ----------------------------- newRequest 986.5 ns/iter 1,013,682.6 (878.2 ns … 1.18 µs) 1.01 µs 1.18 µs 1.18 µs newAbortController 18 ns/iter 55,541,104.1 (15.6 ns … 42.62 ns) 17.71 ns 25.05 ns 26.27 ns newAbortControllerSignal 18.66 ns/iter 53,578,966.7 (16.49 ns … 32.16 ns) 18.71 ns 25.67 ns 26.39 ns newAbortControllerSignalOnAbort 106.49 ns/iter 9,390,164.9 (97.87 ns … 120.61 ns) 108.6 ns 114.24 ns 115.89 ns newAbortControllerSignalAddEventListener 86.92 ns/iter 11,504,880.2 (81.88 ns … 103.15 ns) 90 ns 98.28 ns 99.55 ns newAbortControllerSignalOnAbortNoListener 3.01 µs/iter 331,964.4 (2.97 µs … 3.1 µs) 3.06 µs 3.1 µs 3.1 µs newAbortControllerSignalOnAbortAbort 3.26 µs/iter 306,662.6 (3.22 µs … 3.36 µs) 3.27 µs 3.36 µs 3.36 µs ``` Latest canary: ``` Running 10s test @ http://localhost:8080/ 2 threads and 10 connections Thread Stats Avg Stdev Max +/- Stdev Latency 72.86us 71.23us 4.47ms 99.05% Req/Sec 64.66k 5.54k 72.48k 82.18% 1299015 requests in 10.10s, 185.83MB read Requests/sec: 128616.02 Transfer/sec: 18.40MB cpu: Apple M2 Pro runtime: deno 1.43.0+bc4aa5f (aarch64-apple-darwin) file:///Users/matt/Documents/scripts/bench_request.js benchmark time (avg) iter/s (min … max) p75 p99 p995 ----------------------------------------------------------------------------------------------- ----------------------------- newRequest 1.25 µs/iter 800,005.2 (1.01 µs … 4.18 µs) 1.16 µs 4.18 µs 4.18 µs newAbortController 18.56 ns/iter 53,868,204.3 (16.04 ns … 38.73 ns) 18.38 ns 26.1 ns 26.63 ns newAbortControllerSignal 18.72 ns/iter 53,430,746.1 (16.13 ns … 36.71 ns) 18.71 ns 26.19 ns 26.98 ns newAbortControllerSignalOnAbort 193.91 ns/iter 5,156,992.4 (184.25 ns … 211.41 ns) 194.96 ns 207.87 ns 209.4 ns newAbortControllerSignalAddEventListener 171.45 ns/iter 5,832,569.2 (153 ns … 182.03 ns) 176.17 ns 180.75 ns 181.05 ns newAbortControllerSignalOnAbortNoListener 3.07 µs/iter 326,263.3 (2.98 µs … 3.17 µs) 3.08 µs 3.17 µs 3.17 µs newAbortControllerSignalOnAbortAbort 3.32 µs/iter 301,344.6 (3.29 µs … 3.4 µs) 3.33 µs 3.4 µs 3.4 µs ``` --- ext/fetch/23_request.js | 65 ++++++++++++++++++++++++++------------ ext/fetch/internal.d.ts | 1 - ext/http/00_serve.ts | 32 +++++++------------ ext/http/01_http.js | 10 +++--- ext/web/03_abort_signal.js | 33 ++++++++++--------- 5 files changed, 79 insertions(+), 62 deletions(-) diff --git a/ext/fetch/23_request.js b/ext/fetch/23_request.js index 61c3b3f5d5..70e00a874e 100644 --- a/ext/fetch/23_request.js +++ b/ext/fetch/23_request.js @@ -14,6 +14,7 @@ const { ArrayPrototypeMap, ArrayPrototypeSlice, ArrayPrototypeSplice, + ObjectFreeze, ObjectKeys, ObjectPrototypeIsPrototypeOf, RegExpPrototypeExec, @@ -24,7 +25,6 @@ const { } = primordials; import * as webidl from "ext:deno_webidl/00_webidl.js"; -import { assert } from "ext:deno_web/00_infra.js"; import { createFilteredInspectProxy } from "ext:deno_console/01_console.js"; import { byteUpperCase, @@ -43,8 +43,12 @@ import { headersFromHeaderList, } from "ext:deno_fetch/20_headers.js"; import { HttpClientPrototype } from "ext:deno_fetch/22_http_client.js"; -import * as abortSignal from "ext:deno_web/03_abort_signal.js"; - +import { + createDependentAbortSignal, + newSignal, + signalAbort, +} from "ext:deno_web/03_abort_signal.js"; +import { DOMException } from "ext:deno_web/01_dom_exception.js"; const { internalRidSymbol } = core; const _request = Symbol("request"); @@ -52,6 +56,7 @@ const _headers = Symbol("headers"); const _getHeaders = Symbol("get headers"); const _headersCache = Symbol("headers cache"); const _signal = Symbol("signal"); +const _signalCache = Symbol("signalCache"); const _mimeType = Symbol("mime type"); const _body = Symbol("body"); const _url = Symbol("url"); @@ -262,7 +267,13 @@ class Request { } /** @type {AbortSignal} */ - [_signal]; + get [_signal]() { + const signal = this[_signalCache]; + if (signal !== undefined) { + return signal; + } + return (this[_signalCache] = newSignal()); + } get [_mimeType]() { const values = getDecodeSplitHeader( headerListFromHeaders(this[_headers]), @@ -363,11 +374,10 @@ class Request { // 28. this[_request] = request; - // 29. - const signals = signal !== null ? [signal] : []; - - // 30. - this[_signal] = abortSignal.createDependentAbortSignal(signals, prefix); + // 29 & 30. + if (signal !== null) { + this[_signalCache] = createDependentAbortSignal([signal], prefix); + } // 31. this[_headers] = headersFromHeaderList(request.headerList, "request"); @@ -473,17 +483,21 @@ class Request { } const clonedReq = cloneInnerRequest(this[_request]); - assert(this[_signal] !== null); - const clonedSignal = abortSignal.createDependentAbortSignal( - [this[_signal]], + const materializedSignal = this[_signal]; + const clonedSignal = createDependentAbortSignal( + [materializedSignal], prefix, ); - return fromInnerRequest( - clonedReq, - clonedSignal, - guardFromHeaders(this[_headers]), - ); + const request = new Request(_brand); + request[_request] = clonedReq; + request[_signalCache] = clonedSignal; + request[_getHeaders] = () => + headersFromHeaderList( + clonedReq.headerList, + guardFromHeaders(this[_headers]), + ); + return request; } [SymbolFor("Deno.privateCustomInspect")](inspect, inspectOptions) { @@ -562,19 +576,30 @@ function toInnerRequest(request) { /** * @param {InnerRequest} inner - * @param {AbortSignal} signal * @param {"request" | "immutable" | "request-no-cors" | "response" | "none"} guard * @returns {Request} */ -function fromInnerRequest(inner, signal, guard) { +function fromInnerRequest(inner, guard) { const request = new Request(_brand); request[_request] = inner; - request[_signal] = signal; request[_getHeaders] = () => headersFromHeaderList(inner.headerList, guard); return request; } +const signalAbortError = new DOMException( + "The request has been cancelled.", + "AbortError", +); +ObjectFreeze(signalAbortError); + +function abortRequest(request) { + if (request[_signal]) { + request[_signal][signalAbort](signalAbortError); + } +} + export { + abortRequest, fromInnerRequest, newInnerRequest, processUrlList, diff --git a/ext/fetch/internal.d.ts b/ext/fetch/internal.d.ts index e0137c59dc..17565992f4 100644 --- a/ext/fetch/internal.d.ts +++ b/ext/fetch/internal.d.ts @@ -70,7 +70,6 @@ declare module "ext:deno_fetch/26_fetch.js" { function toInnerRequest(request: Request): InnerRequest; function fromInnerRequest( inner: InnerRequest, - signal: AbortSignal | null, guard: | "request" | "immutable" diff --git a/ext/http/00_serve.ts b/ext/http/00_serve.ts index afcc16f38b..b12a873905 100644 --- a/ext/http/00_serve.ts +++ b/ext/http/00_serve.ts @@ -49,7 +49,11 @@ import { ResponsePrototype, toInnerResponse, } from "ext:deno_fetch/23_response.js"; -import { fromInnerRequest, toInnerRequest } from "ext:deno_fetch/23_request.js"; +import { + abortRequest, + fromInnerRequest, + toInnerRequest, +} from "ext:deno_fetch/23_request.js"; import { AbortController } from "ext:deno_web/03_abort_signal.js"; import { _eventLoop, @@ -126,8 +130,6 @@ function addTrailers(resp, headerList) { op_http_set_response_trailers(inner.external, headerList); } -let signalAbortError; - class InnerRequest { #external; #context; @@ -137,14 +139,13 @@ class InnerRequest { #upgraded; #urlValue; #completed; - #abortController; + request; - constructor(external, context, abortController) { + constructor(external, context) { this.#external = external; this.#context = context; this.#upgraded = false; this.#completed = undefined; - this.#abortController = abortController; } close(success = true) { @@ -158,15 +159,7 @@ class InnerRequest { ); } } - if (!signalAbortError) { - signalAbortError = new DOMException( - "The request has been cancelled.", - "AbortError", - ); - } - // Unconditionally abort the request signal. Note that we don't use - // an error here. - this.#abortController.abort(signalAbortError); + abortRequest(this.request); this.#external = null; } @@ -492,17 +485,16 @@ function fastSyncResponseOrStream( */ function mapToCallback(context, callback, onError) { return async function (req) { - const abortController = new AbortController(); - const signal = abortController.signal; - // Get the response from the user-provided callback. If that fails, use onError. If that fails, return a fallback // 500 error. let innerRequest; let response; try { - innerRequest = new InnerRequest(req, context, abortController); + innerRequest = new InnerRequest(req, context); + const request = fromInnerRequest(innerRequest, "immutable"); + innerRequest.request = request; response = await callback( - fromInnerRequest(innerRequest, signal, "immutable"), + request, new ServeHandlerInfo(innerRequest), ); diff --git a/ext/http/01_http.js b/ext/http/01_http.js index 580ba11666..b547682891 100644 --- a/ext/http/01_http.js +++ b/ext/http/01_http.js @@ -38,10 +38,10 @@ import { toInnerResponse, } from "ext:deno_fetch/23_response.js"; import { + abortRequest, fromInnerRequest, newInnerRequest, } from "ext:deno_fetch/23_request.js"; -import { AbortController } from "ext:deno_web/03_abort_signal.js"; import { _eventLoop, _idleTimeoutDuration, @@ -147,19 +147,17 @@ class HttpConn { body !== null ? new InnerBody(body) : null, false, ); - const abortController = new AbortController(); const request = fromInnerRequest( innerRequest, - abortController.signal, "immutable", false, ); const respondWith = createRespondWith( this, + request, readStreamRid, writeStreamRid, - abortController, ); return { request, respondWith }; @@ -200,9 +198,9 @@ class HttpConn { function createRespondWith( httpConn, + request, readStreamRid, writeStreamRid, - abortController, ) { return async function respondWith(resp) { try { @@ -384,7 +382,7 @@ function createRespondWith( ws[_serverHandleIdleTimeout](); } } catch (error) { - abortController.abort(error); + abortRequest(request); throw error; } finally { if (deleteManagedResource(httpConn, readStreamRid)) { diff --git a/ext/web/03_abort_signal.js b/ext/web/03_abort_signal.js index 4971fa4188..053b89bdf9 100644 --- a/ext/web/03_abort_signal.js +++ b/ext/web/03_abort_signal.js @@ -7,8 +7,8 @@ import { primordials } from "ext:core/mod.js"; const { ArrayPrototypeEvery, ArrayPrototypePush, + FunctionPrototypeApply, ObjectPrototypeIsPrototypeOf, - SafeArrayIterator, SafeSet, SafeSetIterator, SafeWeakRef, @@ -82,6 +82,14 @@ const timerId = Symbol("[[timerId]]"); const illegalConstructorKey = Symbol("illegalConstructorKey"); class AbortSignal extends EventTarget { + [abortReason] = undefined; + [abortAlgos] = null; + [dependent] = false; + [sourceSignals] = null; + [dependentSignals] = null; + [timerId] = null; + [webidl.brand] = webidl.brand; + static any(signals) { const prefix = "Failed to execute 'AbortSignal.any'"; webidl.requiredArguments(arguments.length, 1, prefix); @@ -141,9 +149,11 @@ class AbortSignal extends EventTarget { const algos = this[abortAlgos]; this[abortAlgos] = null; - const event = new Event("abort"); - setIsTrusted(event, true); - super.dispatchEvent(event); + if (listenerCount(this, "abort") > 0) { + const event = new Event("abort"); + setIsTrusted(event, true); + super.dispatchEvent(event); + } if (algos !== null) { for (const algorithm of new SafeSetIterator(algos)) { algorithm(); @@ -168,13 +178,6 @@ class AbortSignal extends EventTarget { throw new TypeError("Illegal constructor."); } super(); - this[abortReason] = undefined; - this[abortAlgos] = null; - this[dependent] = false; - this[sourceSignals] = null; - this[dependentSignals] = null; - this[timerId] = null; - this[webidl.brand] = webidl.brand; } get aborted() { @@ -199,8 +202,8 @@ class AbortSignal extends EventTarget { // `[add]` and `[remove]` don't ref and unref the timer because they can // only be used by Deno internals, which use it to essentially cancel async // ops which would block the event loop. - addEventListener(...args) { - super.addEventListener(...new SafeArrayIterator(args)); + addEventListener() { + FunctionPrototypeApply(super.addEventListener, this, arguments); if (listenerCount(this, "abort") > 0) { if (this[timerId] !== null) { refTimer(this[timerId]); @@ -216,8 +219,8 @@ class AbortSignal extends EventTarget { } } - removeEventListener(...args) { - super.removeEventListener(...new SafeArrayIterator(args)); + removeEventListener() { + FunctionPrototypeApply(super.removeEventListener, this, arguments); if (listenerCount(this, "abort") === 0) { if (this[timerId] !== null) { unrefTimer(this[timerId]);