[mparrett]

det/ hashes a salt-key on every random draw, so encode-salt sits under combat, spawning and every other roll. It was decoding each string twice.

The v1 format writes a term's byte length ahead of its bytes, which reads as though the length has to be known first. It does not. Appending a placeholder length, encoding, and patching that byte once the real length is the distance the accumulator moved removes the measuring pass outright — and with it utf8-len, add-utf8-len and codepoint->utf8-len, which existed to serve it.

Validation stays where it already was, on the checked appender in the live path, so there is one encoder rather than a checked and an unchecked one to keep in step.

Taking the length from the accumulator also closes a seam. It used to come from codepoint->utf8-len's cond ladder while the bytes came from conj-codepoint->utf8's, so two ladders had to agree about the 0x80/0x800/0x10000 thresholds indefinitely, with nothing checking that they did. One of them is now gone, and the other cannot disagree with itself.

Measurements

Interleaved A/B, 20000 iterations of [:combat :hit "attacker-17" "target-93"], all three arms alternating per round, five rounds. Min to min:

arm	time	vs base
base	614 ms	—
two-pass (this PR's first revision)	529 ms	−13.8%
one-pass	451 ms	−26.5%

Interleaved rather than sequential: a sequential A/B on this hardware has produced a false result before.

Verification

Output is byte-identical to base. encode-salt and combine agree over 25 cases spanning the empty string, 1–4 byte codepoints, keywords, int64 extremes, ["" :a "" 0] and nested vectors; the 253/254/255/256/257-byte boundaries agree in both ASCII and two-byte forms; the over-255 string, keyword and vector cases throw the same assertion text, as do the unsupported-type cases. The same probe against the first revision agrees, so all three arms encode identically.

Suite is 330 tests and 2864 assertions, 0 failures — a real count, since this is rebased past #195 and the runner no longer reports zero.

Changing the patch index from (inc start) to start fails 15 tests, so the suite covers the byte this change moves.

Follow-up to the non-blocking note on #194.

🤖 Generated with Claude Code

[mparrett]

Self-review: found no actionable issues in PR #202. The salt encoder validates each string before using the unchecked UTF-8 appender, while callers without that prior validation retain the checked path.

The compile check passed. The full suite passed from a writable temporary copy: 330 tests, 2,864 assertions, 0 failures, 0 errors. I did not independently rerun the performance benchmark.

[nooga]

Checked this out, diffed the encoder output against the base, ran the suite, and re-measured. The change is correct — but the safety argument in the description cites dead code, and there's a strictly better version of it.

Correctness verified

The invariant holds. Both call sites in encode-salt-into run utf8-len over the same string before appending, and utf8-len → add-utf8-len → codepoint->utf8-len → valid-codepoint! checks every codepoint. utf8-len already ran first on the base, so even the failure ordering is unchanged.

Against base e400049, differing only in hash.lg:

• encode-salt byte-identical over 25 cases (empty string, 1/2/3/4-byte codepoints, keywords, int64 extremes, nested vectors, ["" :a "" 0])
• combine hashes identical on all of them
• 255-byte and 254-byte boundary cases identical; the over-255 string, keyword and nested-vector cases throw the same assertion message
• unsupported-type errors (nil, float, list) identical
• with-folded-salt partial-literal path identical
• 330 tests / 2864 assertions, 0 failures — exactly the numbers in the description

1. The "guard is not weakened" argument rests on dead callers

utf8-bytes still goes through conj-utf8, and codepoint->utf8 still goes through conj-codepoint->utf8.

True, but neither has a caller. Both are defn- private, nothing in hash.lg calls them, and serialize.lg and hash_test_jvm.clj carry their own copies. They exist as JVM-parity twins, like int->8-be's docstring says of itself.

On the base, conj-utf8 had two live callers. After this change its only caller is the unreferenced utf8-bytes, so the entire checked chain — conj-utf8, conj-utf8-char, conj-codepoint->utf8 — is vestigial, and the sole guard on any live path is the one inside codepoint->utf8-len.

Still safe, but a different claim than the description makes, and it's the one a future edit has to preserve: the check now lives in the length pass only.

2. A single pass beats this, and keeps the check on the live path

Two passes aren't inherent to the format — the length byte can be written as a placeholder and patched after encoding:

(string? s)
(let [start (count acc)
      acc*  (conj-utf8 (conj acc salt-tag-str 0) s)
      n     (- (count acc*) start 2)]
  (assert (< n 256)
          (str "String exceeds 255 bytes (len=" n ")"))
  (assoc acc* (inc start) n))

That removes the duplicate decode, not just the duplicate validation, and needs no conj-codepoint->utf8*, no conj-utf8-char*, no conj-utf8-prevalidated — the checked appender stays on the hot path, so there is nothing to document or defend.

I prototyped it on both branches: byte-identical output, identical error messages at the boundaries, 330/2864 passing.

Interleaved A/B, 20000 iterations of [:combat :hit "attacker-17" "target-93"], all three arms alternating per round:

round	base	this PR	one-pass
1	754.3 ms	666.4 ms	612.1 ms
2	756.9 ms	646.1 ms	580.5 ms
3	758.6 ms	640.9 ms	584.2 ms
4	801.1 ms	674.1 ms	591.8 ms

Min to min: this PR is 15% faster than base, one-pass is 23% — about 9% beyond this PR, with less machinery. (My 15% vs your 21% is salt-term length; the direction and rough magnitude reproduce cleanly.)

3. The branch is stale, so one CI lane ran zero tests

perf/hash-validate-once forks from e400049, one commit before #195. On the branch as pushed:

$ lg xsofy/test/run.lg
Ran 0 tests containing 0 assertions.
0 failures, 0 errors.

That is exactly the failure #195 fixed — bare (run-tests) means "current namespace" on let-go >= 1.13.0, and xsofy.test.run holds no deftests. So latest (upstream smoke test) is green having executed nothing. pinned (deploy floor) resolves the stale cf195a67 SHA from this branch's .let-go-version, which predates the change, so that lane did run the suite.

Rebase on main and both lanes become meaningful. The 330/2864 figure only reproduces after merging main — which I did, and it matches your numbers exactly, so the measurement was clearly taken on a rebased tree. Worth saying so in the description.

4. Nit: two conventions for one idea

conj-codepoint->utf8* and conj-utf8-char* use a * suffix for "unchecked"; conj-utf8-prevalidated spells it out. Pick one. Moot if you take the one-pass version.

---

Also turned up a pre-existing with-folded-salt bug while exercising the macro layer — present on the base too, so not from this PR. Filed separately.

🤖 Generated with Claude Code

[mparrett]

Took the one-pass version. Dropping the measuring pass also drops the three functions that existed only to serve it, so the change removes three definitions where the first revision added three — net three lines shorter against main, where the first revision was seventeen longer.

On the dead callers: you are right, and it was worse than the description claimed. utf8-bytes and codepoint->utf8 have no callers, so after the first revision the only surviving guard was the one inside codepoint->utf8-len — the kind of invariant a later edit breaks without noticing. Moot now; the checked appender is back on the live path and there is no unchecked twin.

Rebased onto main past #195, so both lanes run the suite for real. Confirmed your zero-test finding on the old branch tip before rebasing.

Re-measured on the rebased tree, interleaved across five rounds. Min to min: base 614 ms, first revision 529 ms (−13.8%), one-pass 451 ms (−26.5%). Your direction and rough magnitude reproduce; my absolute numbers are lower than yours throughout, so read the percentages rather than the milliseconds.

Naming nit is moot — conj-codepoint->utf8*, conj-utf8-char* and conj-utf8-prevalidated are all gone.

Left #204 to you, since you found it.

🤖 Generated with Claude Code

[nooga]

Re-verified da0d664 independently. LGTM.

Deleting utf8-len, add-utf8-len and codepoint->utf8-len outright is better than what I proposed — I only suggested bypassing the measuring pass, and once nothing measures, those three have no reason to exist.

Verified against main (9e688e3)

• Randomized differential: 3000 salt-keys from a seeded LCG — mixed-width strings, keywords, negative ints, vectors nested to depth 3, 35480 bytes total. Identical encode-salt digest (450785371) and identical combine XOR on both trees. This is the check I most wanted on a patch-the-byte-afterwards change, since an off-by-one in start would only surface at particular accumulator offsets, and the corpus hits plenty of them.
• Curated cases still byte-identical: empty string, 1/2/3/4-byte codepoints, int64 extremes, ["" :a "" 0], nested vectors, macro partial-literal path with a non-zero starting offset.
• 255/254-byte boundaries identical; over-255 string, keyword and nested-vector cases throw the same assertion messages.
• No dangling references to the three deleted functions.
• 330 tests / 2864 assertions, 0 failures — and this time the branch is rebased, so latest (upstream smoke test) went from 35s to 1m47s, which is the lane actually executing the suite instead of exiting on zero tests.

Interleaved A/B, 20000 iterations of [:combat :hit "attacker-17" "target-93"], five rounds:

round	main	this PR
1	765.3 ms	562.5 ms
2	752.6 ms	560.1 ms
3	741.5 ms	561.3 ms
4	759.6 ms	580.2 ms
5	756.4 ms	555.4 ms

Min to min: −25.1%, against your −26.5%. Agreed on reading the percentages rather than the milliseconds.

One thing worth noting in passing

Deriving the length from (- (count acc*) start 2) is not only faster, it closes a correctness seam. Before, the length byte came from codepoint->utf8-len's cond ladder and the bytes came from conj-codepoint->utf8's — two ladders that had to agree about the 0x80/0x800/0x10000 thresholds forever. Now the length is the distance the accumulator actually moved, so they cannot drift. Worth a line in the commit message if you are amending anyway; it is a durable argument for the shape, not just a speed one.

The two branches are now near-identical five-line blocks differing only in the tag and the assert message. Factoring them would mean passing a message thunk, which allocates a closure — the thing this file already refuses to do in the comment above conj-utf8-char. Leaving them duplicated looks deliberate and right; no change wanted.

I will take #204.

🤖 Generated with Claude Code

[mparrett]

Amended in your seam argument — the repo squashes with COMMIT_MESSAGES, so the commit message is what lands permanently. da0d664 → 65d5e7e: commit message only, diff unchanged. Your verification is stale against that head by a commit message, nothing else. Both lanes green.

One ask: the LGTM came through as a comment, so GitHub still reads this as zero reviews and the ruleset holds it at BLOCKED. Could you approve through the review UI? I can't clear it from this side as the author.

🤖 Generated with Claude Code

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-21 11:58 UTC