diff --git a/src/bun.js/bindings/ZigSourceProvider.cpp b/src/bun.js/bindings/ZigSourceProvider.cpp index 4bb245f310..d3afd80811 100644 --- a/src/bun.js/bindings/ZigSourceProvider.cpp +++ b/src/bun.js/bindings/ZigSourceProvider.cpp @@ -104,8 +104,11 @@ Ref SourceProvider::create(Zig::GlobalObject* globalObject, Reso if (resolvedSource.needsDeref && !isBuiltin) { resolvedSource.source_code.deref(); - resolvedSource.specifier.deref(); - // source_url gets deref'd by the WTF::String above. + + // Do not deref either source_url or specifier + // Specifier's lifetime is the JSValue, mostly + // source_url is owned by the string above + // https://github.com/oven-sh/bun/issues/9521 } auto provider = adoptRef(*new SourceProvider( diff --git a/src/bun.js/bindings/exports.zig b/src/bun.js/bindings/exports.zig index ee3a563a7f..99ee9ce599 100644 --- a/src/bun.js/bindings/exports.zig +++ b/src/bun.js/bindings/exports.zig @@ -184,10 +184,18 @@ pub const ResolvedSource = extern struct { pub const name = "ResolvedSource"; pub const namespace = shim.namespace; + /// Specifier's lifetime is the caller from C++ + /// https://github.com/oven-sh/bun/issues/9521 specifier: bun.String = bun.String.empty, source_code: bun.String = bun.String.empty, + + /// source_url is eventually deref'd on success source_url: bun.String = bun.String.empty, + + // this pointer is unused and shouldn't exist commonjs_exports: ?[*]ZigString = null, + + // This field is used to indicate whether it's a CommonJS module or ESM commonjs_exports_len: u32 = 0, hash: u32 = 0, @@ -195,7 +203,9 @@ pub const ResolvedSource = extern struct { allocator: ?*anyopaque = null, tag: Tag = Tag.javascript, - needs_deref: bool = true, + + /// This is for source_code + source_code_needs_deref: bool = true, pub const Tag = @import("ResolvedSourceTag").ResolvedSourceTag; }; diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index 2229aff9ef..09596f71db 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -1557,7 +1557,7 @@ pub const VirtualMachine = struct { .source_url = bun.String.init(source_url), .hash = 0, .allocator = null, - .needs_deref = false, + .source_code_needs_deref = false, }; } var source = this.refCountedString(code, hash_, !add_double_ref); @@ -1572,7 +1572,7 @@ pub const VirtualMachine = struct { .source_url = bun.String.init(source_url), .hash = source.hash, .allocator = source, - .needs_deref = false, + .source_code_needs_deref = false, }; } @@ -1626,36 +1626,36 @@ pub const VirtualMachine = struct { return builtin; } - var virtual_source: ?*logger.Source = null; - - var display_specifier = _specifier.toUTF8(bun.default_allocator); + const display_specifier = _specifier.toUTF8(bun.default_allocator); defer display_specifier.deinit(); - var specifier_clone = _specifier.toUTF8(bun.default_allocator); + const specifier_clone = _specifier.toUTF8(bun.default_allocator); defer specifier_clone.deinit(); var display_slice = display_specifier.slice(); const specifier = ModuleLoader.normalizeSpecifier(jsc_vm, specifier_clone.slice(), &display_slice); const referrer_clone = referrer.toUTF8(bun.default_allocator); defer referrer_clone.deinit(); const path = Fs.Path.init(specifier_clone.slice()); - var loader = jsc_vm.bundler.options.loaders.get(path.name.ext) orelse brk: { - if (strings.eqlLong(specifier, jsc_vm.main, true)) { - break :brk options.Loader.js; + const loader, const virtual_source = brk: { + if (jsc_vm.module_loader.eval_source) |eval_source| { + if (strings.endsWithComptime(specifier, bun.pathLiteral("/[eval]"))) { + break :brk .{ .tsx, eval_source }; + } + if (strings.endsWithComptime(specifier, bun.pathLiteral("/[stdin]"))) { + break :brk .{ .tsx, eval_source }; + } } - break :brk options.Loader.file; + break :brk .{ + jsc_vm.bundler.options.loaders.get(path.name.ext) orelse brk2: { + if (strings.eqlLong(specifier, jsc_vm.main, true)) { + break :brk2 options.Loader.js; + } + break :brk2 options.Loader.file; + }, + null, + }; }; - if (jsc_vm.module_loader.eval_source) |eval_source| { - if (strings.endsWithComptime(specifier, bun.pathLiteral("/[eval]"))) { - virtual_source = eval_source; - loader = .tsx; - } - if (strings.endsWithComptime(specifier, bun.pathLiteral("/[stdin]"))) { - virtual_source = eval_source; - loader = .tsx; - } - } - // .print_source, which is used by exceptions avoids duplicating the entire source code // but that means we have to be careful of the lifetime of the source code // so we only want to reset the arena once its done freeing it. diff --git a/src/bun.js/module_loader.zig b/src/bun.js/module_loader.zig index c1507ad3a2..72dbddc32d 100644 --- a/src/bun.js/module_loader.zig +++ b/src/bun.js/module_loader.zig @@ -144,7 +144,7 @@ inline fn jsSyntheticModule(comptime name: ResolvedSource.Tag, specifier: String .source_url = bun.String.init(@tagName(name)), .hash = 0, .tag = name, - .needs_deref = false, + .source_code_needs_deref = false, }; } @@ -312,12 +312,18 @@ pub const RuntimeTranspilerStore = struct { const promise = this.promise.swap(); const globalThis = this.globalThis; this.poll_ref.unref(vm); - var specifier = if (this.parse_error == null) this.resolved_source.specifier else bun.String.createUTF8(this.path.text); + const referrer = bun.String.createUTF8(this.referrer); var log = this.log; this.log = logger.Log.init(bun.default_allocator); var resolved_source = this.resolved_source; - resolved_source.source_url = specifier.dupeRef(); + const specifier = brk: { + if (this.parse_error != null) { + break :brk bun.String.createUTF8(this.path.text); + } + + break :brk resolved_source.specifier; + }; resolved_source.tag = brk: { if (resolved_source.commonjs_exports_len > 0) { @@ -544,7 +550,7 @@ pub const RuntimeTranspilerStore = struct { }, }, .specifier = duped, - .source_url = if (duped.eqlUTF8(path.text)) duped.dupeRef() else String.init(path.text), + .source_url = duped.createIfDifferent(path.text), .hash = 0, .commonjs_exports_len = if (entry.metadata.module_type == .cjs) std.math.maxInt(u32) else 0, }; @@ -558,7 +564,7 @@ pub const RuntimeTranspilerStore = struct { .allocator = null, .source_code = bun.String.createLatin1(parse_result.source.contents), .specifier = duped, - .source_url = if (duped.eqlUTF8(path.text)) duped.dupeRef() else String.init(path.text), + .source_url = duped.createIfDifferent(path.text), .hash = 0, }; return; @@ -646,7 +652,7 @@ pub const RuntimeTranspilerStore = struct { break :brk result; }, .specifier = duped, - .source_url = if (duped.eqlUTF8(path.text)) duped.dupeRef() else String.createUTF8(path.text), + .source_url = duped.createIfDifferent(path.text), .commonjs_exports = null, .commonjs_exports_len = if (parse_result.ast.exports_kind == .cjs) std.math.maxInt(u32) @@ -1666,7 +1672,7 @@ pub const ModuleLoader = struct { .allocator = null, .source_code = bun.String.createUTF8(parse_result.source.contents), .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, .tag = ResolvedSource.Tag.json_for_object_loader, @@ -1682,7 +1688,7 @@ pub const ModuleLoader = struct { else => @compileError("unreachable"), }, .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, }; } @@ -1692,7 +1698,7 @@ pub const ModuleLoader = struct { .allocator = null, .source_code = bun.String.createLatin1(parse_result.source.contents), .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, }; @@ -1720,7 +1726,7 @@ pub const ModuleLoader = struct { }, }, .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, .commonjs_exports_len = if (entry.metadata.module_type == .cjs) std.math.maxInt(u32) else 0, .tag = brk: { @@ -1870,7 +1876,7 @@ pub const ModuleLoader = struct { break :brk result; }, .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .commonjs_exports = if (commonjs_exports.len > 0) commonjs_exports.ptr else @@ -1924,7 +1930,7 @@ pub const ModuleLoader = struct { // .allocator = if (jsc_vm.has_loaded) &jsc_vm.allocator else null, // .source_code = ZigString.init(jsc_vm.allocator.dupe(u8, parse_result.source.contents) catch unreachable), // .specifier = ZigString.init(specifier), - // .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + // .source_url = input_specifier.createIfDifferent(path.text), // .hash = 0, // .tag = ResolvedSource.Tag.wasm, // }; @@ -1949,7 +1955,7 @@ pub const ModuleLoader = struct { .allocator = null, .source_code = bun.String.static(@embedFile("../js/wasi-runner.js")), .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .tag = .esm, .hash = 0, }; @@ -2009,7 +2015,7 @@ pub const ModuleLoader = struct { .allocator = null, .source_code = bun.String.createUTF8(sqlite_module_source_code_string), .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .tag = .esm, .hash = 0, }; @@ -2038,7 +2044,7 @@ pub const ModuleLoader = struct { .allocator = &jsc_vm.allocator, .source_code = public_url, .specifier = input_specifier, - .source_url = if (input_specifier.eqlUTF8(path.text)) input_specifier.dupeRef() else String.init(path.text), + .source_url = input_specifier.createIfDifferent(path.text), .hash = 0, }; }, @@ -2281,7 +2287,7 @@ pub const ModuleLoader = struct { .source_url = specifier, .hash = 0, .tag = .esm, - .needs_deref = true, + .source_code_needs_deref = true, }; }, @@ -2386,7 +2392,7 @@ pub const ModuleLoader = struct { .specifier = specifier, .source_url = specifier.dupeRef(), .hash = 0, - .needs_deref = false, + .source_code_needs_deref = false, }; } @@ -2396,7 +2402,7 @@ pub const ModuleLoader = struct { .specifier = specifier, .source_url = specifier.dupeRef(), .hash = 0, - .needs_deref = false, + .source_code_needs_deref = false, }; } } diff --git a/src/string.zig b/src/string.zig index b0b29b9c92..a0fb5660fe 100644 --- a/src/string.zig +++ b/src/string.zig @@ -308,6 +308,16 @@ pub const String = extern struct { } } + pub fn createIfDifferent(other: String, utf8_slice: []const u8) String { + if (other.tag == .WTFStringImpl) { + if (other.eqlUTF8(utf8_slice)) { + return other.dupeRef(); + } + } + + return createUTF8(utf8_slice); + } + fn createUninitializedLatin1(len: usize) struct { String, []u8 } { std.debug.assert(len > 0); const string = BunString__fromLatin1Unitialized(len); diff --git a/test/js/bun/resolve/an-empty-file-with-a-strange-extension.weird b/test/js/bun/resolve/an-empty-file-with-a-strange-extension.weird new file mode 100644 index 0000000000..5e40c08770 --- /dev/null +++ b/test/js/bun/resolve/an-empty-file-with-a-strange-extension.weird @@ -0,0 +1 @@ +asdf \ No newline at end of file diff --git a/test/js/bun/resolve/load-file-loader-a-lot.test.ts b/test/js/bun/resolve/load-file-loader-a-lot.test.ts new file mode 100644 index 0000000000..a640f34166 --- /dev/null +++ b/test/js/bun/resolve/load-file-loader-a-lot.test.ts @@ -0,0 +1,25 @@ +import { test, expect } from "bun:test"; + +test("a file: loader file can be imported 10,000 times", async () => { + const prev = Bun.unsafe.gcAggressionLevel(); + Bun.unsafe.gcAggressionLevel(0); + var baseline; + Bun.gc(true); + for (let i = 0; i < 1000; i++) { + await import("./an-empty-file-with-a-strange-extension.weird?j" + i); + Loader.registry.clear(); + } + baseline = process.memoryUsage.rss(); + Bun.gc(true); + for (let i = 0; i < 100_000; i++) { + await import("./an-empty-file-with-a-strange-extension.weird?i" + i); + Loader.registry.clear(); + } + + Bun.gc(true); + const memory = process.memoryUsage.rss(); + Bun.unsafe.gcAggressionLevel(prev); + + // It's pretty hard to test for not leaking specifier strings correctly. We need string stats. + console.log("Memory usage: ", (memory - baseline) / 1024 / 1024, "MB"); +}, 10_000); diff --git a/test/js/bun/resolve/load-same-empty-js-file-a-lot.js b/test/js/bun/resolve/load-same-empty-js-file-a-lot.js new file mode 100644 index 0000000000..6452be66f4 --- /dev/null +++ b/test/js/bun/resolve/load-same-empty-js-file-a-lot.js @@ -0,0 +1,2 @@ +// do not use reference import.meta here. +export default {}; diff --git a/test/js/bun/resolve/load-same-js-file-a-lot.js b/test/js/bun/resolve/load-same-js-file-a-lot.js new file mode 100644 index 0000000000..39fc3c3b8f --- /dev/null +++ b/test/js/bun/resolve/load-same-js-file-a-lot.js @@ -0,0 +1,9 @@ +// make sure an import.meta is available +export default { + url: import.meta.url.toLocaleLowerCase(), + dir: import.meta.dir.toLocaleLowerCase(), + file: import.meta.file.toLocaleLowerCase(), + path: import.meta.path.toLocaleLowerCase(), + dirname: import.meta.dirname.toLocaleLowerCase(), + filename: import.meta.filename.toLocaleLowerCase(), +}; diff --git a/test/js/bun/resolve/load-same-js-file-a-lot.test.ts b/test/js/bun/resolve/load-same-js-file-a-lot.test.ts new file mode 100644 index 0000000000..9f9d864373 --- /dev/null +++ b/test/js/bun/resolve/load-same-js-file-a-lot.test.ts @@ -0,0 +1,39 @@ +import { test, expect } from "bun:test"; +import { withoutAggressiveGC } from "harness"; + +test("load the same file 10,000 times", async () => { + const meta = { + url: import.meta.url.toLocaleLowerCase().replace(".test.ts", ".js"), + dir: import.meta.dir.toLocaleLowerCase().replace(".test.ts", ".js"), + file: import.meta.file.toLocaleLowerCase().replace(".test.ts", ".js"), + path: import.meta.path.toLocaleLowerCase().replace(".test.ts", ".js"), + dirname: import.meta.dirname.toLocaleLowerCase().replace(".test.ts", ".js"), + filename: import.meta.filename.toLocaleLowerCase().replace(".test.ts", ".js"), + }; + const prev = Bun.unsafe.gcAggressionLevel(); + Bun.unsafe.gcAggressionLevel(0); + for (let i = 0; i < 10000; i++) { + const { + default: { url, dir, file, path, dirname, filename }, + } = await import("./load-same-js-file-a-lot.js?i=" + i); + expect(url).toBe(meta.url + "?i=" + i); + expect(dir).toBe(meta.dir); + expect(file).toBe(meta.file); + expect(path).toBe(meta.path); + expect(dirname).toBe(meta.dirname); + expect(filename).toBe(meta.filename); + } + Bun.gc(true); + Bun.unsafe.gcAggressionLevel(prev); +}); + +test("load the same empty JS file 10,000 times", async () => { + const prev = Bun.unsafe.gcAggressionLevel(); + Bun.unsafe.gcAggressionLevel(0); + for (let i = 0; i < 10000; i++) { + const { default: obj } = await import("./load-same-empty-js-file-a-lot.js?i=" + i); + expect(obj).toEqual({}); + } + Bun.gc(true); + Bun.unsafe.gcAggressionLevel(prev); +}); diff --git a/test/js/bun/util/text-loader-fixture-dynamic-import-stress.ts b/test/js/bun/util/text-loader-fixture-dynamic-import-stress.ts new file mode 100644 index 0000000000..0eb376132d --- /dev/null +++ b/test/js/bun/util/text-loader-fixture-dynamic-import-stress.ts @@ -0,0 +1,8 @@ +const count = process.platform === "win32" ? 10_000 : 100_000; +for (let i = 0; i < count; i++) { + await import("./text-loader-fixture-text-file.txt?" + i++); +} + +const { default: text } = await import("./text-loader-fixture-text-file.txt"); + +console.write(text); diff --git a/test/js/bun/util/text-loader.test.ts b/test/js/bun/util/text-loader.test.ts index f7d3b2a284..65e6f23ed4 100644 --- a/test/js/bun/util/text-loader.test.ts +++ b/test/js/bun/util/text-loader.test.ts @@ -5,6 +5,7 @@ import { join } from "path"; describe("text-loader", () => { const fixtures = [ + ["dynamic-import reloaded 10000 times", "text-loader-fixture-dynamic-import-stress.ts"], ["dynamic-import", "text-loader-fixture-dynamic-import.ts"], ["import", "text-loader-fixture-import.ts"], ["require", "text-loader-fixture-require.ts"], @@ -20,6 +21,10 @@ describe("text-loader", () => { stdin: "ignore", }); + if (result.exitCode !== 0) { + console.log({ result }); + } + expect(result.stdout.toString()).toBe("These are words!"); expect(result.exitCode).toBe(0); }); diff --git a/test/js/third_party/svelte/bun-loader-svelte.ts b/test/js/third_party/svelte/bun-loader-svelte.ts index f0a6e3419a..c30a7bd11e 100644 --- a/test/js/third_party/svelte/bun-loader-svelte.ts +++ b/test/js/third_party/svelte/bun-loader-svelte.ts @@ -6,11 +6,14 @@ await plugin({ var { compile } = await import("svelte/compiler"); var { readFileSync } = await import("fs"); await 2; - builder.onLoad({ filter: /\.svelte$/ }, ({ path }) => ({ - contents: compile(readFileSync(path, "utf8"), { - filename: path, - generate: "ssr", - }).js.code, + builder.onLoad({ filter: /\.svelte(\?[^.]+)?$/ }, ({ path }) => ({ + contents: compile( + readFileSync(path.substring(0, path.includes("?") ? path.indexOf("?") : path.length), "utf-8"), + { + filename: path, + generate: "ssr", + }, + ).js.code, loader: "js", })); await 1; diff --git a/test/js/third_party/svelte/svelte.test.ts b/test/js/third_party/svelte/svelte.test.ts index 44e36cce27..67167ecbe0 100644 --- a/test/js/third_party/svelte/svelte.test.ts +++ b/test/js/third_party/svelte/svelte.test.ts @@ -8,9 +8,30 @@ describe("require", () => { expect(html).toBe("

Hello world!

"); }); + + it("works if you require it 1,000 times", () => { + const prev = Bun.unsafe.gcAggressionLevel(); + Bun.unsafe.gcAggressionLevel(0); + for (let i = 0; i < 1000; i++) { + const { default: App } = require("./hello.svelte?r" + i); + expect(App.render).toBeFunction(); + } + Bun.gc(true); + Bun.unsafe.gcAggressionLevel(prev); + }); }); describe("dynamic import", () => { + it("works if you import it 1,000 times", async () => { + const prev = Bun.unsafe.gcAggressionLevel(); + Bun.unsafe.gcAggressionLevel(0); + for (let i = 0; i < 1000; i++) { + const { default: App } = await import("./hello.svelte?i" + i); + expect(App.render).toBeFunction(); + } + Bun.gc(true); + Bun.unsafe.gcAggressionLevel(prev); + }); it("SSRs `

Hello world!

` with Svelte", async () => { const { default: App }: any = await import("./hello.svelte");