From 9ecae59bbb1a302977afa94fd879a0c6f8d6195f Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 30 Jul 2023 23:51:43 -0700 Subject: Fix memory leak in response.clone(), further reduce memory usage of Request & Response (#3902) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Atomize respsone.url & response.statusText * Fix warning * Atomize Request & Response URLs when possible * Fix memory leak in response.clone() bun/bench/snippets on  jarred/atomize ❯ mem bun --smol request-response-clone.mjs cpu: Apple M1 Max runtime: bun 0.7.2 (arm64-darwin) benchmark time (avg) (min … max) p75 p99 p995 -------------------------------------------------------- ----------------------------- req.clone().url 77.3 ns/iter (40.35 ns … 222.64 ns) 91.53 ns 128.11 ns 172.78 ns resp.clone().url 162.43 ns/iter (116 ns … 337.77 ns) 177.4 ns 232.38 ns 262.65 ns Peak memory usage: 60 MB bun/bench/snippets on  jarred/atomize ❯ mem bun-0.7.1 --smol request-response-clone.mjs cpu: Apple M1 Max runtime: bun 0.7.1 (arm64-darwin) benchmark time (avg) (min … max) p75 p99 p995 -------------------------------------------------------- ----------------------------- req.clone().url 115.85 ns/iter (80.35 ns … 247.39 ns) 128.19 ns 181.93 ns 207.23 ns resp.clone().url 252.32 ns/iter (202.6 ns … 351.07 ns) 266.56 ns 325.88 ns 334.73 ns Peak memory usage: 1179 MB * Update tests * Update js_ast.zig * Update test --------- Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com> --- src/bun.js/api/filesystem_router.zig | 9 ++++--- src/bun.js/api/html_rewriter.zig | 4 +-- src/bun.js/api/server.zig | 47 +++++++++++++++++++++--------------- 3 files changed, 36 insertions(+), 24 deletions(-) (limited to 'src/bun.js/api') diff --git a/src/bun.js/api/filesystem_router.zig b/src/bun.js/api/filesystem_router.zig index 216f66b7f..edb666040 100644 --- a/src/bun.js/api/filesystem_router.zig +++ b/src/bun.js/api/filesystem_router.zig @@ -75,6 +75,8 @@ const DeprecatedGlobalRouter = struct { }; var path_: ?ZigString.Slice = null; + var req_url_slice: ZigString.Slice = .{}; + defer req_url_slice.deinit(); var pathname: string = ""; defer { if (path_) |path| { @@ -88,7 +90,8 @@ const DeprecatedGlobalRouter = struct { var url = URL.parse(path_.?.slice()); pathname = url.pathname; } else if (arg.as(Request)) |req| { - var url = URL.parse(req.url); + req_url_slice = req.url.toUTF8(bun.default_allocator); + var url = URL.parse(req_url_slice.slice()); pathname = url.pathname; } @@ -389,11 +392,11 @@ pub const FileSystemRouter = struct { if (argument.isCell()) { if (argument.as(JSC.WebCore.Request)) |req| { req.ensureURL() catch unreachable; - break :brk ZigString.Slice.fromUTF8NeverFree(req.url).clone(globalThis.allocator()) catch unreachable; + break :brk req.url.toUTF8(globalThis.allocator()); } if (argument.as(JSC.WebCore.Response)) |resp| { - break :brk ZigString.Slice.fromUTF8NeverFree(resp.url).clone(globalThis.allocator()) catch unreachable; + break :brk resp.url.toUTF8(globalThis.allocator()); } } diff --git a/src/bun.js/api/html_rewriter.zig b/src/bun.js/api/html_rewriter.zig index 92e9790c6..f754e3e23 100644 --- a/src/bun.js/api/html_rewriter.zig +++ b/src/bun.js/api/html_rewriter.zig @@ -454,8 +454,8 @@ pub const HTMLRewriter = struct { result.body.init.headers = headers.cloneThis(global); } - result.url = bun.default_allocator.dupe(u8, original.url) catch unreachable; - result.status_text = bun.default_allocator.dupe(u8, original.status_text) catch unreachable; + result.url = original.url.clone(); + result.status_text = original.status_text.clone(); var input = original.body.value.useAsAnyBlob(); sink.input = input; diff --git a/src/bun.js/api/server.zig b/src/bun.js/api/server.zig index 801745aee..10afa4ac1 100644 --- a/src/bun.js/api/server.zig +++ b/src/bun.js/api/server.zig @@ -1069,7 +1069,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp byte_stream: ?*JSC.WebCore.ByteStream = null, /// Used in errors - pathname: []const u8 = "", + pathname: bun.String = bun.String.empty, /// Used either for temporary blob data or fallback /// When the response body is a temporary value @@ -1550,10 +1550,9 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp stream.unpipe(); } - if (this.pathname.len > 0) { - ctxLog("finalizeWithoutDeinit: this.pathname.len > 0 null", .{}); - this.allocator.free(bun.constStrToU8(this.pathname)); - this.pathname = ""; + if (!this.pathname.isEmpty()) { + this.pathname.deref(); + this.pathname = bun.String.empty; } // if we are waiting for the body yet and the request was not aborted we can safely clear the onData callback @@ -2235,7 +2234,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp request_object.uws_request = req; request_object.ensureURL() catch { - request_object.url = ""; + request_object.url = bun.String.empty; }; // we have to clone the request headers here since they will soon belong to a different request @@ -2244,7 +2243,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp } if (comptime debug_mode) { - ctx.pathname = bun.default_allocator.dupe(u8, request_object.url) catch unreachable; + ctx.pathname = request_object.url.clone(); } // This object dies after the stack frame is popped @@ -2596,15 +2595,28 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp runErrorHandlerWithStatusCode(this, value, 500); } - fn ensurePathname(this: *RequestContext) []const u8 { - if (this.pathname.len > 0) - return this.pathname; + const PathnameFormatter = struct { + ctx: *RequestContext, + + pub fn format(formatter: @This(), comptime fmt: []const u8, opts: std.fmt.FormatOptions, writer: anytype) !void { + var this = formatter.ctx; + + if (!this.pathname.isEmpty()) { + try this.pathname.format(fmt, opts, writer); + return; + } - if (!this.flags.has_abort_handler) { - return this.req.url(); + if (!this.flags.has_abort_handler) { + try writer.writeAll(this.req.url()); + return; + } + + try writer.writeAll("/"); } + }; - return "/"; + fn ensurePathname(this: *RequestContext) PathnameFormatter { + return .{ .ctx = this }; } pub inline fn shouldCloseConnection(this: *const RequestContext) bool { @@ -2625,7 +2637,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp vm.log, error.ExceptionOcurred, exception_list.toOwnedSlice() catch @panic("TODO"), - "{s} - {s} failed", + "{s} - {} failed", .{ @as(string, @tagName(this.method)), this.ensurePathname() }, ); } else { @@ -4900,8 +4912,7 @@ pub fn NewServer(comptime ssl_enabled_: bool, comptime debug_mode_: bool) type { } existing_request = Request{ - .url = url.href, - .url_was_allocated = true, + .url = bun.String.create(url.href), .headers = headers, .body = JSC.WebCore.InitRequestBodyValue(body) catch unreachable, .method = method, @@ -4943,7 +4954,7 @@ pub fn NewServer(comptime ssl_enabled_: bool, comptime debug_mode_: bool) type { } if (response_value.as(JSC.WebCore.Response)) |resp| { - resp.url = this.allocator.dupe(u8, existing_request.url) catch unreachable; + resp.url = existing_request.url.clone(); } return JSC.JSPromise.resolvedPromiseValue(ctx, response_value).asObjectRef(); } @@ -5295,7 +5306,6 @@ pub fn NewServer(comptime ssl_enabled_: bool, comptime debug_mode_: bool) type { } request_object.* = .{ - .url = "", .method = ctx.method, .uws_request = req, .https = ssl_enabled, @@ -5398,7 +5408,6 @@ pub fn NewServer(comptime ssl_enabled_: bool, comptime debug_mode_: bool) type { } request_object.* = .{ - .url = "", .method = ctx.method, .uws_request = req, .upgrader = ctx, -- cgit v1.2.3