From 0efc4eaf9776b24c04eba6330a75f43f1f7f4291 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Tue, 18 Feb 2025 19:37:26 -0800 Subject: [PATCH] fix(sql) allow more than 64 columns (#17444) --- src/bun.js/bindings/SQLClient.cpp | 74 ++++++++++----- src/bun.js/bindings/bindings.zig | 8 +- src/main.zig | 2 + src/sql/postgres.zig | 152 +++++++++++++++++++++--------- test/js/sql/sql.test.ts | 21 +++++ 5 files changed, 184 insertions(+), 73 deletions(-) diff --git a/src/bun.js/bindings/SQLClient.cpp b/src/bun.js/bindings/SQLClient.cpp index 9df7cf2f8e..cf6b5692f8 100644 --- a/src/bun.js/bindings/SQLClient.cpp +++ b/src/bun.js/bindings/SQLClient.cpp @@ -27,6 +27,18 @@ namespace Bun { using namespace JSC; +typedef struct ExternColumnIdentifier { + uint8_t tag; + union { + uint32_t index; + BunString name; + }; + + bool isIndexedColumn() const { return tag == 1; } + bool isNamedColumn() const { return tag == 2; } + bool isDuplicateColumn() const { return tag == 0; } +} ExternColumnIdentifier; + typedef struct DataCellArray { struct DataCell* cells; uint32_t length; @@ -286,16 +298,19 @@ static JSC::JSValue toJS(JSC::VM& vm, JSC::JSGlobalObject* globalObject, DataCel } } -static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode) +static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode, ExternColumnIdentifier* namesPtr, uint32_t namesCount) { auto& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); - + std::optional> names = std::nullopt; + if (namesPtr && namesCount > 0) { + names = std::span(namesPtr, namesCount); + } switch (result_mode) { case BunResultMode::Objects: // objects { - auto* object = JSC::constructEmptyObject(vm, structure); + auto* object = structure ? JSC::constructEmptyObject(vm, structure) : JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), 0); // TODO: once we have more tests for this, let's add another branch for // "only mixed names and mixed indexed columns, no duplicates" @@ -317,7 +332,14 @@ static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t co ASSERT(!cell.isDuplicateColumn()); ASSERT(!cell.isIndexedColumn()); ASSERT(cell.isNamedColumn()); - object->putDirectOffset(vm, i, value); + if (names.has_value()) { + + auto name = names.value()[i]; + object->putDirect(vm, Identifier::fromString(vm, name.name.toWTFString()), value); + + } else { + object->putDirectOffset(vm, i, value); + } } } else if (flags.hasIndexedColumns() && !flags.hasNamedColumns() && !flags.hasDuplicateColumns()) { for (uint32_t i = 0; i < count; i++) { @@ -352,7 +374,13 @@ static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t co ASSERT(!cell.isIndexedColumn()); ASSERT(!cell.isDuplicateColumn()); ASSERT(cell.index < count); - object->putDirectOffset(vm, structureOffsetIndex++, value); + + if (names.has_value()) { + auto name = names.value()[structureOffsetIndex++]; + object->putDirect(vm, Identifier::fromString(vm, name.name.toWTFString()), value); + } else { + object->putDirectOffset(vm, structureOffsetIndex++, value); + } } else if (cell.isDuplicateColumn()) { // skip it! } @@ -381,9 +409,9 @@ static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t co return jsUndefined(); } } -static JSC::JSValue toJS(JSC::JSArray* array, JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode) +static JSC::JSValue toJS(JSC::JSArray* array, JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode, ExternColumnIdentifier* namesPtr, uint32_t namesCount) { - JSValue value = toJS(structure, cells, count, globalObject, flags, result_mode); + JSValue value = toJS(structure, cells, count, globalObject, flags, result_mode, namesPtr, namesCount); if (value.isEmpty()) return {}; @@ -403,43 +431,35 @@ static JSC::JSValue toJS(JSC::JSArray* array, JSC::Structure* structure, DataCel extern "C" EncodedJSValue JSC__constructObjectFromDataCell( JSC::JSGlobalObject* globalObject, EncodedJSValue encodedArrayValue, - EncodedJSValue encodedStructureValue, DataCell* cells, uint32_t count, uint32_t flags, uint8_t result_mode) + EncodedJSValue encodedStructureValue, DataCell* cells, uint32_t count, uint32_t flags, uint8_t result_mode, ExternColumnIdentifier* namesPtr, uint32_t namesCount) { JSValue arrayValue = JSValue::decode(encodedArrayValue); JSValue structureValue = JSValue::decode(encodedStructureValue); auto* array = arrayValue ? jsDynamicCast(arrayValue) : nullptr; auto* structure = jsDynamicCast(structureValue); - return JSValue::encode(toJS(array, structure, cells, count, globalObject, Bun::BunStructureFlags(flags), BunResultMode(result_mode))); + return JSValue::encode(toJS(array, structure, cells, count, globalObject, Bun::BunStructureFlags(flags), BunResultMode(result_mode), namesPtr, namesCount)); } -typedef struct ExternColumnIdentifier { - uint8_t tag; - union { - uint32_t index; - BunString name; - }; - - bool isIndexedColumn() const { return tag == 1; } - bool isNamedColumn() const { return tag == 2; } - bool isDuplicateColumn() const { return tag == 0; } -} ExternColumnIdentifier; - -extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject, JSC::JSCell* owner, uint32_t inlineCapacity, ExternColumnIdentifier* namesPtr) +extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject, JSC::JSCell* owner, uint32_t capacity, ExternColumnIdentifier* namesPtr) { auto& vm = JSC::getVM(globalObject); PropertyNameArray propertyNames(vm, PropertyNameMode::Strings, PrivateSymbolMode::Exclude); - std::span names(namesPtr, inlineCapacity); + std::span names(namesPtr, capacity); uint32_t nonDuplicateCount = 0; - for (uint32_t i = 0; i < inlineCapacity; i++) { + + for (uint32_t i = 0; i < capacity; i++) { ExternColumnIdentifier& name = names[i]; if (name.isNamedColumn()) { propertyNames.add(Identifier::fromString(vm, name.name.toWTFString())); } nonDuplicateCount += !name.isDuplicateColumn(); + if (nonDuplicateCount == JSFinalObject::maxInlineCapacity) { + break; + } } - Structure* structure = globalObject->structureCache().emptyObjectStructureForPrototype(globalObject, globalObject->objectPrototype(), std::min(nonDuplicateCount, JSFinalObject::maxInlineCapacity)); + Structure* structure = globalObject->structureCache().emptyObjectStructureForPrototype(globalObject, globalObject->objectPrototype(), nonDuplicateCount); if (owner) { vm.writeBarrier(owner, structure); } else { @@ -450,7 +470,8 @@ extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject if (names.size() > 0) { PropertyOffset offset = 0; uint32_t indexInPropertyNamesArray = 0; - for (uint32_t i = 0; i < inlineCapacity; i++) { + uint32_t propertyNamesSize = propertyNames.size(); + for (uint32_t i = 0; i < capacity && indexInPropertyNamesArray < propertyNamesSize; i++) { ExternColumnIdentifier& name = names[i]; if (name.isNamedColumn()) { structure = structure->addPropertyTransition(vm, structure, propertyNames[indexInPropertyNamesArray++], 0, offset); @@ -476,4 +497,5 @@ extern "C" void JSC__putDirectOffset(JSC::VM* vm, JSC::EncodedJSValue object, ui { JSValue::decode(object).getObject()->putDirectOffset(*vm, offset, JSValue::decode(value)); } +extern "C" uint32_t JSC__JSObject__maxInlineCapacity = JSC::JSFinalObject::maxInlineCapacity; } diff --git a/src/bun.js/bindings/bindings.zig b/src/bun.js/bindings/bindings.zig index 035b613d72..81d14e32e1 100644 --- a/src/bun.js/bindings/bindings.zig +++ b/src/bun.js/bindings/bindings.zig @@ -21,7 +21,7 @@ const String = bun.String; const ErrorableString = JSC.ErrorableString; const JSError = bun.JSError; const OOM = bun.OOM; - +pub extern const JSC__JSObject__maxInlineCapacity: c_uint; pub const JSObject = extern struct { pub const shim = Shimmer("JSC", "JSObject", @This()); const cppFn = shim.cppFn; @@ -100,7 +100,7 @@ pub const JSObject = extern struct { } } - extern fn JSC__createStructure(*JSC.JSGlobalObject, *JSC.JSCell, u32, names: [*]ExternColumnIdentifier, flags: u32) JSC.JSValue; + extern fn JSC__createStructure(*JSC.JSGlobalObject, *JSC.JSCell, u32, names: [*]ExternColumnIdentifier) JSC.JSValue; pub const ExternColumnIdentifier = extern struct { tag: u8 = 0, @@ -122,9 +122,9 @@ pub const JSObject = extern struct { } } }; - pub fn createStructure(global: *JSGlobalObject, owner: JSC.JSValue, length: u32, names: [*]ExternColumnIdentifier, flags: u32) JSValue { + pub fn createStructure(global: *JSGlobalObject, owner: JSC.JSValue, length: u32, names: [*]ExternColumnIdentifier) JSValue { JSC.markBinding(@src()); - return JSC__createStructure(global, owner.asCell(), length, names, flags); + return JSC__createStructure(global, owner.asCell(), length, names); } const InitializeCallback = *const fn (ctx: *anyopaque, obj: *JSObject, global: *JSGlobalObject) callconv(.C) void; diff --git a/src/main.zig b/src/main.zig index 63d0a6fee2..07bf7dd81b 100644 --- a/src/main.zig +++ b/src/main.zig @@ -56,7 +56,9 @@ pub fn main() void { if (Environment.isX64 and Environment.enableSIMD and Environment.isPosix) { bun_warn_avx_missing(@import("./cli/upgrade_command.zig").Version.Bun__githubBaselineURL.ptr); } + bun.StackCheck.configureThread(); + bun.CLI.Cli.start(bun.default_allocator); bun.Global.exit(0); } diff --git a/src/sql/postgres.zig b/src/sql/postgres.zig index 1fc6d0835c..f2c7e920c0 100644 --- a/src/sql/postgres.zig +++ b/src/sql/postgres.zig @@ -3259,10 +3259,31 @@ pub const PostgresSQLConnection = struct { u32, Flags, u8, // result_mode + ?[*]JSC.JSObject.ExternColumnIdentifier, // names + u32, // names count ) JSValue; - pub fn toJS(this: *Putter, globalObject: *JSC.JSGlobalObject, array: JSValue, structure: JSValue, flags: Flags, result_mode: PostgresSQLQueryResultMode) JSValue { - return JSC__constructObjectFromDataCell(globalObject, array, structure, this.list.ptr, @truncate(this.fields.len), flags, @intFromEnum(result_mode)); + pub fn toJS(this: *Putter, globalObject: *JSC.JSGlobalObject, array: JSValue, structure: JSValue, flags: Flags, result_mode: PostgresSQLQueryResultMode, cached_structure: ?PostgresCachedStructure) JSValue { + var names: ?[*]JSC.JSObject.ExternColumnIdentifier = null; + var names_count: u32 = 0; + if (cached_structure) |c| { + if (c.fields) |f| { + names = f.ptr; + names_count = @truncate(f.len); + } + } + + return JSC__constructObjectFromDataCell( + globalObject, + array, + structure, + this.list.ptr, + @truncate(this.fields.len), + flags, + @intFromEnum(result_mode), + names, + names_count, + ); } fn putImpl(this: *Putter, index: u32, optional_bytes: ?*Data, comptime is_raw: bool) !bool { @@ -3446,12 +3467,12 @@ pub const PostgresSQLConnection = struct { const request = this.current() orelse return error.ExpectedRequest; var statement = request.statement orelse return error.ExpectedStatement; var structure: JSValue = .undefined; + var cached_structure: ?PostgresCachedStructure = null; // explict use switch without else so if new modes are added, we don't forget to check for duplicate fields switch (request.flags.result_mode) { .objects => { - // check for duplicate fields - statement.checkForDuplicateFields(); - structure = statement.structure(this.js_value, this.globalObject); + cached_structure = statement.structure(this.js_value, this.globalObject); + structure = cached_structure.?.jsValue() orelse .undefined; }, .raw, .values => { // no need to check for duplicate fields or structure @@ -3466,17 +3487,17 @@ pub const PostgresSQLConnection = struct { .globalObject = this.globalObject, }; - var stack_buf: [64]DataCell = undefined; - var cells: []DataCell = stack_buf[0..@min(statement.fields.len, stack_buf.len)]; + var stack_buf: [70]DataCell = undefined; + var cells: []DataCell = stack_buf[0..@min(statement.fields.len, JSC.JSC__JSObject__maxInlineCapacity)]; + var free_cells = false; defer { for (cells[0..putter.count]) |*cell| { cell.deinit(); } + if (free_cells) bun.default_allocator.free(cells); } - var free_cells = false; - defer if (free_cells) bun.default_allocator.free(cells); - if (statement.fields.len >= 64) { + if (statement.fields.len >= JSC.JSC__JSObject__maxInlineCapacity) { cells = try bun.default_allocator.alloc(DataCell, statement.fields.len); free_cells = true; } @@ -3503,7 +3524,7 @@ pub const PostgresSQLConnection = struct { bun.assert(thisValue != .zero); const pending_value = PostgresSQLQuery.pendingValueGetCached(thisValue) orelse .zero; pending_value.ensureStillAlive(); - const result = putter.toJS(this.globalObject, pending_value, structure, statement.fields_flags, request.flags.result_mode); + const result = putter.toJS(this.globalObject, pending_value, structure, statement.fields_flags, request.flags.result_mode, cached_structure); if (pending_value == .zero) { PostgresSQLQuery.pendingValueSetCached(thisValue, this.globalObject, result); @@ -3906,8 +3927,39 @@ pub const PostgresSQLConnection = struct { } }; +pub const PostgresCachedStructure = struct { + structure: JSC.Strong = .{}, + // only populated if more than JSC.JSC__JSObject__maxInlineCapacity fields otherwise the structure will contain all fields inlined + fields: ?[]JSC.JSObject.ExternColumnIdentifier = null, + + pub fn has(this: *@This()) bool { + return this.structure.has() or this.fields != null; + } + + pub fn jsValue(this: *const @This()) ?JSC.JSValue { + return this.structure.get(); + } + + pub fn set(this: *@This(), globalObject: *JSC.JSGlobalObject, value: ?JSC.JSValue, fields: ?[]JSC.JSObject.ExternColumnIdentifier) void { + if (value) |v| { + this.structure.set(globalObject, v); + } + this.fields = fields; + } + + pub fn deinit(this: *@This()) void { + this.structure.deinit(); + if (this.fields) |fields| { + this.fields = null; + for (fields) |*name| { + name.deinit(); + } + bun.default_allocator.free(fields); + } + } +}; pub const PostgresSQLStatement = struct { - cached_structure: JSC.Strong = .{}, + cached_structure: PostgresCachedStructure = .{}, ref_count: u32 = 1, fields: []protocol.FieldDescription = &[_]protocol.FieldDescription{}, parameters: []const int4 = &[_]int4{}, @@ -4024,43 +4076,57 @@ pub const PostgresSQLStatement = struct { bun.default_allocator.destroy(this); } - pub fn structure(this: *PostgresSQLStatement, owner: JSValue, globalObject: *JSC.JSGlobalObject) JSValue { - return this.cached_structure.get() orelse { - const ids = bun.default_allocator.alloc(JSC.JSObject.ExternColumnIdentifier, this.fields.len) catch return .undefined; - this.checkForDuplicateFields(); - defer { - for (ids) |*name| { - name.deinit(); - } - bun.default_allocator.free(ids); - } + pub fn structure(this: *PostgresSQLStatement, owner: JSValue, globalObject: *JSC.JSGlobalObject) PostgresCachedStructure { + if (this.cached_structure.has()) { + return this.cached_structure; + } + this.checkForDuplicateFields(); - for (this.fields, ids) |*field, *id| { - id.tag = switch (field.name_or_index) { - .name => 2, - .index => 1, - .duplicate => 0, - }; - switch (field.name_or_index) { - .name => |name| { - id.value.name = String.createUTF8(name.slice()); - }, - .index => |index| { - id.value.index = index; - }, - .duplicate => {}, - } + // lets avoid most allocations + var stack_ids: [70]JSC.JSObject.ExternColumnIdentifier = undefined; + // lets de duplicate the fields early + var nonDuplicatedCount = this.fields.len; + for (this.fields) |*field| { + if (field.name_or_index == .duplicate) { + nonDuplicatedCount -= 1; } - const structure_ = JSC.JSObject.createStructure( + } + const ids = if (nonDuplicatedCount <= JSC.JSC__JSObject__maxInlineCapacity) stack_ids[0..nonDuplicatedCount] else bun.default_allocator.alloc(JSC.JSObject.ExternColumnIdentifier, nonDuplicatedCount) catch bun.outOfMemory(); + + var i: usize = 0; + for (this.fields) |*field| { + if (field.name_or_index == .duplicate) continue; + + var id: *JSC.JSObject.ExternColumnIdentifier = &ids[i]; + switch (field.name_or_index) { + .name => |name| { + id.value.name = String.createAtomIfPossible(name.slice()); + }, + .index => |index| { + id.value.index = index; + }, + .duplicate => unreachable, + } + id.tag = switch (field.name_or_index) { + .name => 2, + .index => 1, + .duplicate => 0, + }; + i += 1; + } + + if (nonDuplicatedCount > JSC.JSC__JSObject__maxInlineCapacity) { + this.cached_structure.set(globalObject, null, ids); + } else { + this.cached_structure.set(globalObject, JSC.JSObject.createStructure( globalObject, owner, @truncate(ids.len), ids.ptr, - @bitCast(this.fields_flags), - ); - this.cached_structure.set(globalObject, structure_); - return structure_; - }; + ), null); + } + + return this.cached_structure; } }; diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index df61b4ca65..574b47c1a3 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -174,6 +174,27 @@ if (isDockerEnabled()) { expect(result).toBe(1); }); + describe("should work with more than the max inline capacity", () => { + for (let size of [50, 60, 62, 64, 70, 100]) { + for (let duplicated of [true, false]) { + test(`${size} ${duplicated ? "+ duplicated" : "unique"} fields`, async () => { + const longQuery = `select ${Array.from({ length: size }, (_, i) => { + if (duplicated) { + return i % 2 === 0 ? `${i + 1} as f${i}, ${i} as f${i}` : `${i} as f${i}`; + } + return `${i} as f${i}`; + }).join(",\n")}`; + const result = await sql.unsafe(longQuery); + let value = 0; + for (const column of Object.values(result[0])) { + expect(column).toBe(value); + value++; + } + }); + } + } + }); + test("Connection timeout works", async () => { const onclose = mock(); const onconnect = mock();