perf: decode ASCII varchar values without iconv

[arthurschreiber]

Summary

Decoding a varchar / char value currently always goes through iconv.decode, which canonicalizes the encoding name, looks up the codec, allocates a fresh decoder object, allocates an intermediate buffer of twice the input size, and finally converts that to a string. This PR reworks that path:

1. ASCII fast path. For the overwhelmingly common case — pure ASCII data — iconv is skipped entirely and the bytes are decoded with the native latin1 conversion. Two guards keep this safe for every collation:

• Per-value: isAscii(data) (from node:buffer, native) — only pure 7-bit data takes the fast path.
• Per-codepage: the codepage must be in a hardcoded list of codepages known to decode the 7-bit ASCII range identically to ASCII. The set of codepages that can reach readChars is closed — they all come from the collation tables in collation.ts — and every one of them is ASCII-transparent. A codepage missing from the list only loses the fast path; decoding still works correctly via iconv. A test verifies the ASCII-transparency of every collation-table codepage against iconv itself, so a future collation addition that violates this assumption fails the suite.

2. Native UTF-8 decoding. Non-ASCII UTF-8 values (SQL 2019+ _UTF8 collations) are decoded with the native Buffer#toString('utf8') instead of iconv.

3. Decoder reuse for the remaining iconv path. Non-ASCII values in the single- and multi-byte codepages keep one decoder per codepage (via iconv.getDecoder(codepage, { stripBOM: false })) instead of letting iconv.decode create a fresh one per value. This is safe because decoding a value is fully synchronous and these decoders are back in their initial state after write + end — SBCS decoders are stateless, DBCS decoders explicitly reset their state in end(), and stripBOM: false avoids iconv's BOM wrapper, the only decoder layer that carries state across values.

4. The PLP paths share the same helper. varchar(max) values (row, NBC row, and return-value parsers) previously called iconv.decode directly; they now go through the same decodeChars helper and get the same performance and semantics.

Behavior change: leading byte order marks are preserved

Current tedious silently strips a leading U+FEFF from varchar values under UTF-8 collations — an accident of iconv.decode's document-oriented stripBOM: true default. That character is part of the stored data, not an encoding marker:

• SQL Server counts it: a value inserted as NCHAR(0xFEFF) + N'hello' into a varchar(100) ... _UTF8 column reports LEN = 6, DATALENGTH = 8 (verified against SQL Server 2025).
• tedious returns the same value intact from an nvarchar column, but with the first character silently deleted from the varchar(UTF8) column.
• SqlClient (UTF8Encoding.GetString) and mssql-jdbc (new String(bytes, UTF_8)) both preserve it.

This PR stops stripping it, making varchar(UTF8) consistent with nvarchar and with other drivers. This is a user-visible behavior change for values that genuinely start with U+FEFF (e.g. data bulk-loaded from BOM-prefixed files) — they now round-trip losslessly.

Internal UTF-8 fallback encodings are standardized on the 'utf-8' spelling produced by the collation tables (previously a mix of 'utf8' and 'utf-8').

Performance

• ASCII values: ~460ns → ~100ns per short value (~4.5x).
• Non-ASCII UTF-8 values: ~600ns → ~240ns per value through readValue (~2.4x).
• Non-ASCII single/multi-byte values (decoder reuse): ~15% faster for CP1252, ~20% for CP932/Shift-JIS.
• End-to-end (repeated SELECTs of 2,000-row result sets against a real SQL Server, median of 5 runs, verified in both execution orders):
  • varchar-heavy table (2 ASCII varchar columns): ~405k → ~600k rows/sec (+45–55%)
  • mixed ORM-style table (1 varchar column out of 8): ~243k → ~284k rows/sec (+17%)

Correctness

• Differential fuzz, zero mismatches across both suites:
  • 54,000 values (pure ASCII, random bytes, high-byte-heavy data full of truncated multi-byte sequences) decoded sequentially across 18 codepages against the current implementation — sequential decoding specifically exercises the decoder-reuse path, so state leaking between values would surface here.
  • 51,000 values (adding BOM-prefixed and truncated-UTF-8 categories) across all 17 collation codepages against the intended semantics (iconv.decode(data, cp, { stripBOM: false })) — pinning that native toString('utf8') matches iconv's replacement-character behavior exactly, including for malformed input.
• New unit tests cover: ASCII values across single-byte, multi-byte, and UTF-8 codepages; the ASCII-transparency invariant for every collation-table codepage; non-ASCII single-byte (CP1252), multi-byte (Shift-JIS), and UTF-8 (incl. astral plane) values; BOM preservation on repeated values (both the inline and varchar(max) PLP paths); malformed UTF-8 matching iconv's behavior; decoder state isolation (a value ending in a truncated Shift-JIS sequence followed by a clean value); a codepage where ASCII bytes do not decode as ASCII (UTF-16BE); empty values; NULLs; char values; and an almost-ASCII value.
• Full unit suite: 444 passing.

Notes

• An earlier revision of this branch preserved the BOM-stripping behavior and reused the UTF-8 decoder through the cache; that combination was subtly wrong because iconv's StripBOMWrapper only strips a BOM once per decoder lifetime. Investigating that led to the realization that the stripping itself was unintended behavior, resolved as described above.

🤖 Generated with Claude Code

[claude]

Review: perf: decode ASCII varchar values without iconv

Nicely scoped, well-motivated optimization. The core idea — probe once per codepage whether it's ASCII-identical, then take a native latin1 fast path per-value guarded by isAscii — is sound and avoids hardcoding assumptions about which codepages are "safe."

Correctness

• The empirical probe (iconv.decode of all 128 ASCII bytes vs. latin1) is a good approach — it self-corrects if iconv-lite's codec tables ever change, and the utf-16be test confirms the gate actually rejects incompatible codepages rather than just working by luck.
• latin1 decoding of bytes 0–127 is byte-identical to ASCII, so the fast path is safe wherever the gate passes.
• The try/catch around the probe (defaulting to false on an invalid codepage) preserves existing behavior — an invalid codepage still throws later from the iconv.decode fallback exactly as it did before this change, just skips the fast path first.
• Confirmed metadata.collation!.codepage! is always sourced from the two fixed, bounded lookup tables in src/collation.ts (~17 known codepages) plus the 'utf-8'/'utf8' literals, so the module-level asciiCompatibleCodepages cache is bounded and not a memory-growth concern.

Minor / non-blocking observations

• codepage ?? DEFAULT_ENCODING is now evaluated twice in readChars (once for isAsciiCompatible, once for the iconv.decode fallback). Since all current call sites pass a non-null codepage via !, this is dead code inherited from the pre-existing implementation rather than something new — not asking for a change, just noting it's not new dead code introduced by this PR.
• The cache uses the codepage string as-is (e.g., 'CP1252' vs. a hypothetical 'cp1252' would be treated as distinct entries). Not an actual bug today since codepage strings all come from the fixed tables with consistent casing, but worth keeping in mind if codepage values are ever sourced from less controlled input in the future.
• The known gap (PLP / varchar(max) path in row-token-parser.ts still calling iconv.decode directly) is called out explicitly in the PR description as a deliberate follow-up — reasonable to keep this PR small.

Test coverage

Good breadth: pure-ASCII across single-byte/multi-byte/UTF-8 codepages, non-ASCII single-byte and multi-byte fallthrough, the UTF-16BE "ASCII bytes decode differently" pin test, empty values, NULL, fixed-length char, and an "ASCII except last byte" case that exercises the per-value isAscii scan rather than just the per-codepage gate. The differential fuzz test mentioned in the description (36k values × 18 codepages) is a strong correctness signal, though it isn't part of the committed test suite — that's fine for a manual validation step, just noting it isn't something CI will re-run on future refactors of this code.

Security

No new attack surface — codepage strings still come from the same trusted, bounded internal tables as before; nothing here takes user-controlled codepage input.

Overall: solid, well-tested perf improvement with a safe, self-verifying compatibility check rather than a hardcoded codepage list. No blocking issues found.

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.88%. Comparing base (12f27ce) to head (a8f8e85).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/token/nbcrow-token-parser.ts	0.00%	1 Missing ⚠️
src/token/returnvalue-token-parser.ts	0.00%	1 Missing ⚠️
src/token/row-token-parser.ts	0.00%	0 Missing and 1 partial ⚠️
src/value-parser.ts	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1755      +/-   ##
==========================================
+ Coverage   80.84%   80.88%   +0.03%     
==========================================
  Files          90       90              
  Lines        4913     4902      -11     
  Branches      924      928       +4     
==========================================
- Hits         3972     3965       -7     
+ Misses        645      640       -5     
- Partials      296      297       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Review: perf: decode ASCII varchar values without iconv

Nice, well-documented optimization with strong test/benchmark backing. A few notes:

Code quality & correctness

• The asciiCompatibleCodepages set matches exactly the Encoding union type in src/collation.ts (CP437, CP850, CP874, CP932, CP936, CP949, CP950, CP1250–CP1258, plus 'utf-8'), plus 'utf8' for the DEFAULT_ENCODING fallback used when Collation.codepage is undefined (unrecognized sortId/languageId). Good catch including that fallback case.
• readChars is called from all three relevant sites (VarChar/Char, Text, and the NVarChar/... no—just the char paths), so the fast path applies everywhere it should. readNChars (nvarchar/ntext) is correctly left untouched since those are always UTF-16.
• Decoding ASCII bytes via .toString('latin1') is correct — codepoints 0–127 are identical between ASCII and Latin-1.
• Minor nit: codepage ?? DEFAULT_ENCODING is computed twice (once in the if, once in the fallback iconv.decode call). Hoisting it into a local const encoding = codepage ?? DEFAULT_ENCODING at the top of readChars would avoid the duplication and read slightly cleaner — purely stylistic, not a correctness issue.

Test coverage

• Good breadth: ASCII across single/multi-byte/UTF-8 codepages, non-ASCII single- and multi-byte fallthrough, the UTF-16BE "ASCII bytes decode differently" pin test, empty/NULL/char, and the almost-ASCII-except-last-byte case.
• The should decode ASCII bytes natively for every codepage a collation can produce test is a nice invariant check, but it's worth calling out its limits explicitly: it verifies every codepage that is in the collation tables is ASCII-transparent via iconv, but it does not assert that asciiCompatibleCodepages in value-parser.ts stays in sync with codepageByLanguageId/codepageBySortId. If a future collation codepage is added to collation.ts but the developer forgets to add it to the hardcoded set in value-parser.ts, this test suite will still pass (correctness is preserved via the iconv fallback per the PR description) — it'll just silently lose the fast path for that codepage with no signal. Not a blocker given the documented "missing from the list only loses the fast path" tradeoff, but consider whether the test could also assert set equality between asciiCompatibleCodepages and the union of Object.values(codepageByLanguageId/codepageBySortId) (minus the utf8/utf-8 special case) so silent fast-path regressions get caught, not just correctness regressions.

Performance

• Approach (per-value isAscii scan + per-codepage allowlist, native latin1 decode) is sound and the benchmarked wins (4.5x for ASCII varchar decode, +17–55% end-to-end) look credible for the common case. The ~30% microbenchmark penalty for non-ASCII values on the iconv path is a reasonable and well-disclosed tradeoff given it's "invisible end-to-end."

Other

• The PLP (varchar(max)) path in row-token-parser.ts is correctly called out as an intentional follow-up rather than being silently left inconsistent — appreciate that being flagged explicitly in the PR description rather than left implicit.
• No security concerns — this only affects decode-path performance/codepath selection for trusted server-provided data, doesn't change any protocol handling or introduce new attack surface.

Overall this is a solid, low-risk, well-verified performance improvement. The only substantive suggestion is strengthening the codepage-list-sync test as noted above; everything else is minor style polish.

[claude]

Review

Nice change — well-motivated, clearly benchmarked, and the correctness story (ASCII-transparency probed against iconv itself, plus a sequential differential-fuzz run across codepages) is genuinely convincing. A few notes:

Correctness / robustness

Reused decoders aren't cleaned up on a throw (decodeChars, src/value-parser.ts:608-619).

const result = decoder.write(data);
const trailer = decoder.end();

decodersByCodepage is a module-level singleton shared by every Connection in the process. The safety argument in the PR description ("a decoder that has fully consumed its input via write + end is back in its initial state") only holds if end() always runs after write(). If write() ever throws partway through a value (malformed/truncated multi-byte input, a bug in a codec, a future iconv-lite change), end() is skipped and the decoder is left in whatever partial state write() produced. Since the same decoder instance is reused for the next value decoded with that codepage — potentially on an unrelated connection — that next decode could silently produce garbage instead of throwing or being isolated to the failing call, which is a step down from the previous per-call decoder behavior (where a bad decoder was simply discarded).

In practice iconv-lite's decoders are designed to be lenient (replacement chars rather than throwing) so this may never trigger, but given the blast radius (cross-connection data corruption in a DB driver) it seems worth defending against explicitly, e.g.:

try {
  const result = decoder.write(data);
  const trailer = decoder.end();
  return trailer ? result + trailer : result;
} catch (err) {
  decodersByCodepage.delete(codepage); // drop the possibly-corrupted decoder
  throw err;
}

Minor / type-safety

asciiCompatibleCodepages (src/value-parser.ts:579-584) duplicates the codepage list that Encoding in src/collation.ts:1 already expresses, but as a bare Set<string> — a typo or a future codepage added to Encoding/the collation tables won't be caught by the compiler, only (indirectly) by the new "should decode ASCII bytes natively for every codepage..." test, which validates the fast-path behavior is safe but doesn't actually assert that every Encoding member is present in asciiCompatibleCodepages. Since Encoding isn't currently exported, consider exporting it from collation.ts and typing the set as Set<Encoding | 'utf8'> so this becomes a compile-time guarantee instead of relying solely on the runtime test to notice a gap.

Also trivial: 'utf-8' and 'utf8' are cached as two separate decoder entries in decodersByCodepage even though they're the same codec — harmless given the map tops out around ~18 entries, just a note.

Test coverage

The added tests are thorough (ASCII fast path across codepage kinds, non-ASCII single/multi-byte fallback, decoder state isolation across sequential values, the UTF-16BE non-ASCII-transparent gate case, empty/NULL/char values). One gap: nothing exercises decoder reuse across interleaved different codepages (all the multi-codepage tests iterate one codepage at a time), and nothing pins down cache-recovery behavior if a decode throws — worth adding if the try/finally above gets adopted.

Performance / security

Benchmarks are credible and the approach (cheap Set check + native isAscii before falling back to iconv) is sound. No security concerns — codepage is always sourced from the bounded codepageByLanguageId/codepageBySortId lookup tables in collation.ts (never arbitrary wire data), so the decoder cache can't grow unbounded.

Overall this is solid, well-tested work — the decoder-reuse exception-safety point is the only thing I'd want addressed before merging.

[claude]

Review

Nice, well-documented performance PR. I read through src/value-parser.ts and the new tests in test/unit/value-parser-test.ts, and cross-checked the codepage list against src/collation.ts.

Overview

Adds a three-tier fast path for decoding varchar/char values in readChars:

1. Pure-ASCII data in a known ASCII-compatible codepage is decoded with Buffer#toString('latin1'), skipping iconv entirely.
2. Non-ASCII UTF-8 values are decoded natively with Buffer#toString('utf8') plus a manual leading-BOM strip.
3. The remaining single-/multi-byte codepages reuse one iconv decoder per codepage instead of allocating a new one per value.

Strengths

• The asciiCompatibleCodepages set is exhaustive: I diffed it against every codepage reachable from codepageByLanguageId/codepageBySortId in src/collation.ts, and all of them (CP437, CP850, CP874, CP932, CP936, CP949, CP950, CP1250–CP1258) plus both utf-8/utf8 spellings are present. The added test that verifies ASCII-transparency for every collation codepage against iconv itself is a great guard against silent drift if a future collation is added.
• Choosing 'latin1' over 'ascii' for the fast-path decode is the safer choice — Node's 'ascii' encoding masks the high bit, so 'latin1' is a more defensive pairing with the isAscii() guard even though both are equivalent for genuinely 7-bit input.
• The decoder-reuse rationale (synchronous write + end per value, no await in between, so no interleaving is possible even with concurrent async row parsing) is correct and well explained in the comments.
• Explicitly excluding BOM-aware codecs from the decoder cache (and handling UTF-8 BOM-stripping manually, once per value) correctly avoids the stateful-StripBOM-wrapper bug called out in the PR description. Good catch, and good that a regression test (should strip the byte order mark from every UTF-8 value) pins it.
• Test coverage is thorough: ASCII/non-ASCII/multi-byte/UTF-8 paths, BOM handling across repeated values, malformed UTF-8 vs. iconv parity, decoder-state isolation between consecutive truncated/clean values, NULL/empty/char edge cases, and a codepage (utf-16be) that deliberately breaks the ASCII-identity assumption to confirm the guard actually gates on it.

Suggestions (minor, non-blocking)

• src/value-parser.ts:604 — getCodec is obtained via (iconv as unknown as {...}).getCodec, reaching past the public type definitions into an internal-ish API. This is a reasonable trade-off given getCodec is genuinely part of iconv-lite's runtime public API (used to implement getEncoder/getDecoder themselves), but it's worth a one-line note that a future iconv-lite major bump should re-verify getCodec/bomAware still behave this way, since it's not contractually documented.
• decodeChars checks codepage === 'utf-8' || codepage === 'utf8' inline, duplicating a subset of what asciiCompatibleCodepages already encodes. Not a bug, just a small opportunity to factor out a shared UTF8_CODEPAGES set (or similar) if this file grows more encoding-specific branches later.
• The PR notes the PLP (varchar(max)) path in row-token-parser.ts still calls iconv.decode directly and is intentionally left for a follow-up — makes sense to keep this PR scoped, but worth filing a tracking issue so it isn't forgotten, since it's presumably an equally hot path for large text columns.
• Given benchmarks/token-parser/ already exists in the repo, consider adding a small benchmark script there for readChars/varchar decoding so the ~4.5x/2.4x/15-20% improvements claimed in the description are tracked over time instead of living only in the PR description.

Security

No concerns — this only changes how already-received bytes are decoded into JS strings; it doesn't change trust boundaries, and the ASCII/BOM guards are conservative (fast path only taken when provably safe, falling back to the existing iconv behavior otherwise).

Correctness / Risk

The module-level decodersByCodepage cache is process-global (shared across all Connections), which is safe only because decode is fully synchronous with no await between write() and end(). That invariant holds today, but it's a bit of an implicit contract — if readChars/decodeChars is ever refactored to span an await (e.g., streaming decode for very large PLP values in the follow-up mentioned above), the shared decoder would silently produce corrupted output for interleaved values. Might be worth a short comment at the decodersByCodepage declaration site flagging this constraint explicitly for future editors (some of this is already implied by the existing comments, just calling it out as the one invariant that would be easy to accidentally violate later).

Overall this is a well-reasoned, well-tested optimization with good safety guards (per-value ASCII check + per-codepage allowlist + exclusion of BOM-aware codecs from caching) and convincing before/after numbers. Nice work.

[claude]

Review

Nicely scoped, well-documented performance change with strong differential-fuzz and unit-test backing. A few notes below, nothing blocking.

Overview

Replaces the iconv.decode call on every varchar/char value with: (1) a native-latin1 fast path for pure-ASCII data in ASCII-compatible codepages, (2) native Buffer#toString('utf8') for UTF-8 collations, and (3) a per-codepage cached iconv decoder (via write+end) for the remaining single/multi-byte codepages, replacing per-value decoder allocation. The PLP paths (row, nbcrow, returnvalue parsers) are consolidated onto the same decodeChars helper. Also fixes an accidental behavior bug where a leading U+FEFF was silently stripped from varchar(UTF8) values.

Code quality

• asciiCompatibleCodepages is provably exhaustive against the Collation codepage tables (codepageByLanguageId/codepageBySortId in src/collation.ts only ever produce the 16 codepages + 'utf-8' for the UTF8 flag), and the new test (should decode ASCII bytes natively for every codepage a collation can produce) pins that invariant directly against those exported tables rather than a hand-copied list — good, this will actually fail if collation.ts grows a new codepage that isn't ASCII-transparent.
• decodeChars mirrors iconv-lite's own decode() implementation (write + end) but reuses the decoder object instead of allocating one per call — a reasonable, minimal way to get decoder reuse without depending on undocumented iconv-lite internals.
• Good call standardizing on 'utf-8' throughout instead of the previous 'utf8'/'utf-8' mix — reduces the chance of a codepage silently missing the ASCII fast path or the native-UTF-8 branch due to spelling.

Potential issues / things worth double-checking

1. Global decoder cache statefulness — decodersByCodepage is a module-level Map shared across every Connection in the process. The safety argument (decoder is back in its initial state after synchronous write+end, and readChars/decodeChars never await mid-decode) holds for SBCS codepages trivially, and for DBCS codepages (CP932/936/949/950) it relies on end() fully resetting internal lead-byte state — this is exactly what the new "should parse values independently of previously parsed values" test targets, which is good, but it's worth having someone familiar with iconv-lite internals double check end()'s reset behavior across all four DBCS codepages, not just Shift-JIS, since that's the one path this change can't verify by type-checking alone.
2. Test coverage gap for PLP paths in nbcrow/returnvalue parsers — row-token-parser.ts, nbcrow-token-parser.ts, and returnvalue-token-parser.ts all switch to decodeChars identically, but only row-token-parser-test.ts got a new test exercising the UTF-8/BOM-preservation path. nbcrow-token-parser-test.ts and returnvalue-token-parser-test.ts still only cover plain ASCII digit strings, so the BOM-preservation and decoder-reuse behavior for those two parsers isn't directly asserted (only exercised transitively via readValue unit tests). Given the PR description explicitly calls out "BOM preservation ... on the inline and varchar(max) PLP paths" as a tested guarantee, mirroring the new row-token-parser-test.ts case into the other two would close that gap cheaply.
3. Minor: the fallback default when collation/codepage is missing changed from 'utf8' to 'utf-8' in the three token parsers (?? 'utf8' → ?? 'utf-8') — consistent with the stated goal, just flagging that this is also a (very unlikely-to-hit) user-visible behavior change: previously it went through iconv.decode(..., 'utf8') (BOM-stripping), now decodeChars treats 'utf-8' as the native, non-stripping path. This seems intentional and consistent with the rest of the PR, just noting it since it's not mentioned by name in the description.

Performance

Fast-path gating (Set.has + native isAscii) is O(1) and allocation-free before falling back to the slow path, so ASCII-heavy workloads should see no regression risk from the added branch. The reported benchmarks (4.5x for ASCII, 2.4x for UTF-8, 15-20% for decoder reuse, +17-55% end-to-end) look credible given the removed allocations.

Security

No new attack surface — codepage values only ever originate from the fixed internal lookup tables (codepageByLanguageId/codepageBySortId) or the hardcoded 'utf-8'/DEFAULT_ENCODING fallback, never from attacker-controlled strings passed to iconv.getDecoder, so there's no risk of unbounded cache growth or codec-injection via a malicious server response.

Test coverage

Strong overall — new tests cover ASCII/non-ASCII/SBCS/DBCS/UTF-8 (incl. astral plane), malformed UTF-8 matched against iconv's own output, empty/NULL/char values, the ASCII-transparency invariant across every collation codepage, and decoder-state isolation across sequential values. The one gap is the missing PLP-path-specific BOM test for nbcrow/returnvalue parsers noted above.