perf: remove per-query and per-row allocation hotspots - #4486
Conversation
Query.toPacket always serialized the command twice when CLIENT_QUERY_ATTRIBUTES was negotiated (any MySQL 8+ server): a dry run against Packet.MockBuffer() to compute the length, then the real pass. MockBuffer() itself rebuilt a noop-patched Buffer on every call by iterating Buffer.prototype. With no attributes the packet length is known upfront, so the SQL is now encoded straight into the packet buffer in a single pass; the attribute path still uses the dry run but MockBuffer is memoized. Isolated benchmark (wire replay, no server): SELECT 1 command loop goes from 40.7k to ~220k ops/s; every query()/execute() round trip benefits.
The generated binary row parser called wrap(fields[i], packet) for every non-null column of every row, allocating a metadata object plus three closures, even when no typeCast function is configured (the default). This made execute() slower than query() on wide rows. The wrapper is now only emitted inside the typeCast-function branch. e2e, 100 rows x 100 cols via execute(): 599 -> 966 ops/s; execute() is faster than query() again across the matrix.
parseDateTime decoded the value into a string and handed it to the Date-from-string constructor, which is V8's slow parsing path and dominated date-heavy result sets (~62% of client CPU). The wire format 'YYYY-MM-DD HH:MM:SS[.ffffff]' is now parsed as digits straight from the buffer with no intermediate string. Fixed-offset timezones reuse a memoized offset instead of string concatenation and Date-from-string. Values that don't match the standard shape, zero dates and years below 100 (where new Date(y, ...) changes meaning) keep the previous string-based behaviour. Verified value-identical against the old implementation across timezones (local, Z, +05:30, -08:00), fractional seconds, zero dates and range edges. 100k-row DATETIME/TIMESTAMP/DATE scan: +40% (replay), +32% (e2e); with timezone 'Z' the per-value cost drops ~7x.
The lazy catalog/schema/table/orgTable/orgName getters cached their decoded value with Object.defineProperty on the instance. Column definitions are recreated on every query, and the parser cache key reads schema and table, so every query paid two defineProperty calls per column. The value is now cached in a prototype-declared slot, keeping instance shapes stable.
keyFromFields allocated a nested array per field and serialized the whole structure with JSON.stringify on every query. The fixed-size options head keeps JSON.stringify (preserving its normalization of exotic values), while the per-field part is now built directly with length-prefixed strings, which keeps keys collision-free without the throwaway allocations.
buffer.toString() re-normalizes and re-dispatches the encoding on every call, and StringParser.decode additionally ran Buffer.isEncoding per value. For the encodings that appear in row parsing (utf8, latin1, binary, ascii) decode now calls the corresponding slice method directly, with explicit offset clamping to keep toString()'s out-of-range semantics. Runtimes without these methods fall back to the previous path.
Query.prototype.row resolved this._rows[this._resultIndex] and this._fields[this._resultIndex] on every row packet. The current arrays are now cached when the resultset header is read and reused for the whole resultset (shared by Execute through the common prototype methods).
PacketParser allocated a Packet object per protocol packet, one per row on streams of row data (a 1M-row result allocated 1M of them). Complete packets are now delivered through one mutable instance owned by the parser. This changes the onPacket contract: a consumer that keeps a packet past the synchronous callback must clone() it. The only such place in the client is the paused-connection queue, which now clones; the packet parser unit test retains packets and was updated the same way. Large multi-packet payloads keep their freshly assembled Packet. 1M-row scan: +5% rows/s and measurably fewer minor GC pauses.
readInt64JSNumber/readSInt64JSNumber always allocated a Long only to call toNumber(), and readInt64/readSInt64 allocated one even when the value fits a safe integer. The number is now computed directly from the two 32-bit words (verified bit-identical to Long.toNumber over randomized 64-bit values, including the unsafe range); Long is only constructed when the exact decimal string is actually needed.
The promise wrappers captured an Error stack per query/execute call unconditionally, costing roughly 10% of small-query client CPU, even though the trace connection option (default true) exists for exactly this. Capture is now skipped when trace is false; default behaviour is unchanged.
Two-layer benchmark setup used to find and validate the preceding performance commits: - e2e.js: scenario matrix against a real MySQL server (insert loop, 1/100/10k/100k/1M rows, 10/100 columns, dates, query vs execute) with per-process isolation, warmup, latency percentiles, CPU% and GC pause tracking. - capture.js/replay.js: record the raw server byte stream for a query once, then replay it through PacketParser and the command state machine with a stubbed connection - client-only CPU measurement with controllable chunking, no server variance. - profile-summary.js: aggregate --cpu-prof output by self time. - micro-decode.js: candidate implementations for hot primitives. - ANALYSIS.md: methodology, baseline profiles, measured results and remaining opportunities. Generated fixtures and profiles are gitignored.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4486 +/- ##
==========================================
+ Coverage 92.29% 92.45% +0.15%
==========================================
Files 93 93
Lines 15671 15855 +184
Branches 2190 2254 +64
==========================================
+ Hits 14464 14658 +194
+ Misses 1207 1197 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The digit fast path returned Invalid Date for timezone values that are not 'local', 'Z' or a fixed '+HH:MM' offset. The previous implementation concatenated such values onto the datetime string, and V8's lenient parser accepts some timezone abbreviations there (only when fractional seconds are present, e.g. '2020-01-01 13:45:56.123456PST'). Fall back to the string path for unrecognized timezones so behaviour is unchanged. Found by the new parseDateTime coverage tests.
Bumps patch coverage for the paths codecov flagged: - parseDateTime: fractional seconds, fixed and repeated (memoized) timezone offsets, zero/invalid dates, years below 100, non-standard shapes, timezone abbreviations, SQL NULL - each verified against the legacy Date-from-string semantics. - int64 readers: safe/unsafe magnitudes, signed boundaries, JS-number approximation equality with Number(bigint), exact string forms. - StringParser.decode: toString-compatible clamping of out-of-range offsets, non-fast-path Buffer encodings, iconv encodings with and without options. lib/parsers/string.js is at 100% line coverage; all lines added by this branch in lib/packets/packet.js are covered.
|
Pushed two commits addressing the patch-coverage report:
|
handleCompressedPacket closed over the packet and read packet.numPackets after a nextTick plus asynchronous zlib.inflate. The packet parser now reuses its packet instance, so by then the instance may describe a later frame. It happens to be harmless today only because the reused instance's numPackets is invariantly 1 and multi-part payloads get fresh instances, but any shift in that invariant would silently desync the compressed sequence id. numPackets is now read out synchronously before the task is queued. The regression test mutates the reused instance right after the hand-off and fails against the previous code, for both the deflated and the uncompressed branch.
The zero/invalid date paths returned a module-level INVALID_DATE singleton. Date is mutable - setFullYear() on an Invalid Date makes it valid per spec - so a single caller mutating one result would corrupt every later invalid-date value in the process, across all connections, and hand every caller the same object. The text-protocol fast path in this branch extended the reach of that wart; the binary readDateTime path had it before. All invalid-date returns now allocate new Date(NaN); the invalid path is rare so the allocation is irrelevant. Regression tests cover both the text and binary paths, including the mutate-then-reparse scenario.
|
Two more fixes pushed, addressing review findings:
|
The clamping guards missed NaN offsets (NaN comparisons are false, so NaN start or end slipped through to the *Slice methods, which throw 'Index out of range' where buffer.toString returns a string) as well as fractional offsets. No current caller can produce those shapes - readString normalizes undefined and length-encoded lengths are non-negative - but decode is a general utility and should stay a true drop-in for toString. The fast path now runs the same bounds algorithm as Buffer.prototype.toString: negative and NaN starts coerce to 0 via |0, NaN and negative ends coerce to 0 (empty result), fractional offsets truncate, and an undefined end means the buffer length. The test compares decode against buffer.toString across a grid of degenerate offset shapes (NaN, +/-Infinity, negative, fractional, oversized, undefined) for each fast-path encoding.
|
Pushed The new test asserts decode ≡ toString over the full grid of degenerate offsets (NaN, ±Infinity, negative, fractional, oversized, undefined) × fast-path encodings. Spot-checked the hot path is unaffected: 100k×10cols replay unchanged (~2.05M rows/s). |
|
@wellwelwel a bunch of Claude perf related findings. I'll let it sit for a bit, want to review it myself first, but mostly looks OK. I saw you tagging copilot for reviews - wonder what would it find here |
Hey, @sidorares! I was off for a while due to tech events 🙋🏻♂️ About Copilot reviews, I tag it under various conditions, but especially when everything is done by AI, even if I respond humanly, it will be an AI reading what I said, so I use Copilot so that the AIs interact with each other before I act humanly, but also when:
About what Copilot reviews (and how it reviews), from time to time I update the native skill for this based on the contributions the project receives at .github/skills/code-review/SKILL.md ✨ |
Conflicts: lib/packets/packet.js - kept master's ZERO_DATE prefix detection for binary zero dates (#4491) but return a fresh new Date(NaN) instead of the shared INVALID_DATE singleton, which this branch removed (Date is mutable) - dropped the now-unused INVALID_DATE constant
2 participants
Summary
Ten independent performance fixes (one commit each) found by profiling the client against Docker MySQL 8.3/9 across typical workloads: small command/response loops, 1–100 row queries, 10k–1M row scans, 10/100-column rows, and date-heavy results. The last commit adds the benchmark harness (
benchmarks/perf/) and the full write-up (benchmarks/perf/ANALYSIS.md) with methodology, baseline profiles, and negative results.All changes are pure JS with feature-detected fallbacks — no native modules. Full suite (226 files), lint, and typecheck pass; each commit parses standalone for bisectability.
Highlights
Query.toPacketserialized every query twice on MySQL 8+ (query-attributes capability): a dry run againstPacket.MockBuffer(), which itself rebuilt a patched Buffer per call. Now single-pass when no attributes are set.typeCastconfigured — this madeexecute()slower thanquery()on wide rows.new Date(string)(V8 slow path, ~62% of date-heavy scans); now parsed as digits straight from the buffer, value-identical, with string-path fallback for non-standard shapes.Object.defineProperty, parser-cache key without per-fieldJSON.stringify, directutf8Slice/latin1Slicedecode, cached resultset arrays in the row path,Long-free int64 reads, and the promise wrapper now honorstrace: falsefor its per-query stack capture.Measured (isolated wire replay = client CPU only; e2e = Docker MySQL 8.3)
SELECT 1loop (replay)execute()(e2e)perf: reuse a single Packet instance for complete packetschanges the internalPacketParser.onPacketcontract: packets are delivered through one mutable instance, so a consumer retaining a packet past the synchronous callback mustclone()it. The only such place in the client (paused-connection queue) now clones, and the packet-parser unit test was updated accordingly. If external code constructsPacketParserdirectly and retains packets, this is the one commit to scrutinize — happy to drop it or put it behind an option; it's worth ~5% on large scans plus GC relief.Reproducing
See
benchmarks/perf/ANALYSIS.mdfor baseline profiles, per-fix attribution, negative results (manual UTF-8 decode no longer beats native on modern V8;Object.create(config)options is a 2× regression; WASM parser assessment), and ranked remaining opportunities.