From 9cf9a26330edbaa4227437d0e83852bf4a787d97 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 18 Feb 2025 13:42:59 -0800 Subject: [PATCH] Revert "fix(sql) allow more than 64 columns" (#17441) --- src/bun.js/bindings/SQLClient.cpp | 74 ++++++--------- src/bun.js/bindings/bindings.zig | 10 +- src/main.zig | 3 - src/sql/postgres.zig | 148 +++++++++--------------------- test/js/sql/sql.test.ts | 21 ----- 5 files changed, 71 insertions(+), 185 deletions(-) diff --git a/src/bun.js/bindings/SQLClient.cpp b/src/bun.js/bindings/SQLClient.cpp index cf6b5692f8..9df7cf2f8e 100644 --- a/src/bun.js/bindings/SQLClient.cpp +++ b/src/bun.js/bindings/SQLClient.cpp @@ -27,18 +27,6 @@ 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; @@ -298,19 +286,16 @@ 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, ExternColumnIdentifier* namesPtr, uint32_t namesCount) +static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode) { 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 = structure ? JSC::constructEmptyObject(vm, structure) : JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), 0); + auto* object = JSC::constructEmptyObject(vm, structure); // TODO: once we have more tests for this, let's add another branch for // "only mixed names and mixed indexed columns, no duplicates" @@ -332,14 +317,7 @@ static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t co ASSERT(!cell.isDuplicateColumn()); ASSERT(!cell.isIndexedColumn()); ASSERT(cell.isNamedColumn()); - 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); - } + object->putDirectOffset(vm, i, value); } } else if (flags.hasIndexedColumns() && !flags.hasNamedColumns() && !flags.hasDuplicateColumns()) { for (uint32_t i = 0; i < count; i++) { @@ -374,13 +352,7 @@ static JSC::JSValue toJS(JSC::Structure* structure, DataCell* cells, uint32_t co ASSERT(!cell.isIndexedColumn()); ASSERT(!cell.isDuplicateColumn()); ASSERT(cell.index < count); - - 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); - } + object->putDirectOffset(vm, structureOffsetIndex++, value); } else if (cell.isDuplicateColumn()) { // skip it! } @@ -409,9 +381,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, ExternColumnIdentifier* namesPtr, uint32_t namesCount) +static JSC::JSValue toJS(JSC::JSArray* array, JSC::Structure* structure, DataCell* cells, uint32_t count, JSC::JSGlobalObject* globalObject, Bun::BunStructureFlags flags, BunResultMode result_mode) { - JSValue value = toJS(structure, cells, count, globalObject, flags, result_mode, namesPtr, namesCount); + JSValue value = toJS(structure, cells, count, globalObject, flags, result_mode); if (value.isEmpty()) return {}; @@ -431,35 +403,43 @@ 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, ExternColumnIdentifier* namesPtr, uint32_t namesCount) + EncodedJSValue encodedStructureValue, DataCell* cells, uint32_t count, uint32_t flags, uint8_t result_mode) { 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), namesPtr, namesCount)); + return JSValue::encode(toJS(array, structure, cells, count, globalObject, Bun::BunStructureFlags(flags), BunResultMode(result_mode))); } -extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject, JSC::JSCell* owner, uint32_t capacity, ExternColumnIdentifier* namesPtr) +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) { auto& vm = JSC::getVM(globalObject); PropertyNameArray propertyNames(vm, PropertyNameMode::Strings, PrivateSymbolMode::Exclude); - std::span names(namesPtr, capacity); + std::span names(namesPtr, inlineCapacity); uint32_t nonDuplicateCount = 0; - - for (uint32_t i = 0; i < capacity; i++) { + for (uint32_t i = 0; i < inlineCapacity; 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(), nonDuplicateCount); + Structure* structure = globalObject->structureCache().emptyObjectStructureForPrototype(globalObject, globalObject->objectPrototype(), std::min(nonDuplicateCount, JSFinalObject::maxInlineCapacity)); if (owner) { vm.writeBarrier(owner, structure); } else { @@ -470,8 +450,7 @@ extern "C" EncodedJSValue JSC__createStructure(JSC::JSGlobalObject* globalObject if (names.size() > 0) { PropertyOffset offset = 0; uint32_t indexInPropertyNamesArray = 0; - uint32_t propertyNamesSize = propertyNames.size(); - for (uint32_t i = 0; i < capacity && indexInPropertyNamesArray < propertyNamesSize; i++) { + for (uint32_t i = 0; i < inlineCapacity; i++) { ExternColumnIdentifier& name = names[i]; if (name.isNamedColumn()) { structure = structure->addPropertyTransition(vm, structure, propertyNames[indexInPropertyNamesArray++], 0, offset); @@ -497,5 +476,4 @@ 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 371ef41532..035b613d72 100644 --- a/src/bun.js/bindings/bindings.zig +++ b/src/bun.js/bindings/bindings.zig @@ -21,12 +21,10 @@ 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; - // lets make zig know that in comptime and assert at runtime on main.zig - pub const maxInlineCapacity = if (bun.Environment.isDebug) 62 else 64; pub fn toJS(obj: *JSObject) JSValue { return JSValue.fromCell(obj); @@ -102,7 +100,7 @@ pub const JSObject = extern struct { } } - extern fn JSC__createStructure(*JSC.JSGlobalObject, *JSC.JSCell, u32, names: [*]ExternColumnIdentifier) JSC.JSValue; + extern fn JSC__createStructure(*JSC.JSGlobalObject, *JSC.JSCell, u32, names: [*]ExternColumnIdentifier, flags: u32) JSC.JSValue; pub const ExternColumnIdentifier = extern struct { tag: u8 = 0, @@ -124,9 +122,9 @@ pub const JSObject = extern struct { } } }; - pub fn createStructure(global: *JSGlobalObject, owner: JSC.JSValue, length: u32, names: [*]ExternColumnIdentifier) JSValue { + pub fn createStructure(global: *JSGlobalObject, owner: JSC.JSValue, length: u32, names: [*]ExternColumnIdentifier, flags: u32) JSValue { JSC.markBinding(@src()); - return JSC__createStructure(global, owner.asCell(), length, names); + return JSC__createStructure(global, owner.asCell(), length, names, flags); } const InitializeCallback = *const fn (ctx: *anyopaque, obj: *JSObject, global: *JSGlobalObject) callconv(.C) void; diff --git a/src/main.zig b/src/main.zig index 0ed0ad325c..63d0a6fee2 100644 --- a/src/main.zig +++ b/src/main.zig @@ -56,10 +56,7 @@ 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.assert(bun.JSC.JSObject.maxInlineCapacity == bun.JSC.JSC__JSObject__maxInlineCapacity); - 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 4c3cb0b958..1fc6d0835c 100644 --- a/src/sql/postgres.zig +++ b/src/sql/postgres.zig @@ -3259,31 +3259,10 @@ 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, 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, - ); + 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)); } fn putImpl(this: *Putter, index: u32, optional_bytes: ?*Data, comptime is_raw: bool) !bool { @@ -3467,12 +3446,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 => { - cached_structure = statement.structure(this.js_value, this.globalObject); - structure = cached_structure.?.jsValue() orelse .undefined; + // check for duplicate fields + statement.checkForDuplicateFields(); + structure = statement.structure(this.js_value, this.globalObject); }, .raw, .values => { // no need to check for duplicate fields or structure @@ -3487,17 +3466,17 @@ pub const PostgresSQLConnection = struct { .globalObject = this.globalObject, }; - var stack_buf: [JSC.JSObject.maxInlineCapacity]DataCell = undefined; + var stack_buf: [64]DataCell = undefined; var cells: []DataCell = stack_buf[0..@min(statement.fields.len, stack_buf.len)]; - var free_cells = false; defer { for (cells[0..putter.count]) |*cell| { cell.deinit(); } - if (free_cells) bun.default_allocator.free(cells); } - if (statement.fields.len >= JSC.JSObject.maxInlineCapacity) { + var free_cells = false; + defer if (free_cells) bun.default_allocator.free(cells); + if (statement.fields.len >= 64) { cells = try bun.default_allocator.alloc(DataCell, statement.fields.len); free_cells = true; } @@ -3524,7 +3503,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, cached_structure); + const result = putter.toJS(this.globalObject, pending_value, structure, statement.fields_flags, request.flags.result_mode); if (pending_value == .zero) { PostgresSQLQuery.pendingValueSetCached(thisValue, this.globalObject, result); @@ -3927,39 +3906,8 @@ pub const PostgresSQLConnection = struct { } }; -pub const PostgresCachedStructure = struct { - structure: JSC.Strong = .{}, - // only populated if more than 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: PostgresCachedStructure = .{}, + cached_structure: JSC.Strong = .{}, ref_count: u32 = 1, fields: []protocol.FieldDescription = &[_]protocol.FieldDescription{}, parameters: []const int4 = &[_]int4{}, @@ -4076,57 +4024,43 @@ pub const PostgresSQLStatement = struct { bun.default_allocator.destroy(this); } - pub fn structure(this: *PostgresSQLStatement, owner: JSValue, globalObject: *JSC.JSGlobalObject) PostgresCachedStructure { - if (this.cached_structure.has()) { - return this.cached_structure; - } - this.checkForDuplicateFields(); - - // lets avoid most allocations - var stack_ids: [JSC.JSObject.maxInlineCapacity]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; + 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); } - } - const ids = if (nonDuplicatedCount <= 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, + 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 => {}, + } } - id.tag = switch (field.name_or_index) { - .name => 2, - .index => 1, - .duplicate => 0, - }; - i += 1; - } - - if (nonDuplicatedCount > JSC.JSObject.maxInlineCapacity) { - this.cached_structure.set(globalObject, null, ids); - } else { - this.cached_structure.set(globalObject, JSC.JSObject.createStructure( + const structure_ = JSC.JSObject.createStructure( globalObject, owner, @truncate(ids.len), ids.ptr, - ), null); - } - - return this.cached_structure; + @bitCast(this.fields_flags), + ); + this.cached_structure.set(globalObject, structure_); + return structure_; + }; } }; diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 574b47c1a3..df61b4ca65 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -174,27 +174,6 @@ 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();