Skip to content

fix(bit_hash): off-by-one in sha1_compute padding; restore one-shot FFI#91

Merged
mizchi merged 1 commit into
mainfrom
claude/eloquent-bell-B6IAk
May 31, 2026
Merged

fix(bit_hash): off-by-one in sha1_compute padding; restore one-shot FFI#91
mizchi merged 1 commit into
mainfrom
claude/eloquent-bell-B6IAk

Conversation

@mizchi
Copy link
Copy Markdown
Member

@mizchi mizchi commented May 31, 2026

Summary

moon benchdocs/benchmarks.md のベースラインと比較したら SHA-1 native が ~70-80% 退化 していたので、moon-pprof を使うつもりで環境整備しながら原因を追って修正しました。

真因

modules/bit_hash/src/sha1_ni.c:324 の padding off-by-one:

if (remainder < 55) {  // ← 55 を外している
  /* One padding block. */

remainder == 550x80 (1 byte) + 8-byte length がちょうど現ブロックに収まる境界です。この条件だと 55 が 2-block パスに流れ、その block の内容が前ブロックと同じ data 領域を含んで Sha1State の正しいインクリメンタル計算と違うダイジェストを返していました。

c25986d ("fix: use pure MoonBit SHA-1 path on native to fix key lookup in HubStore") の症状 (HubStore の get_recordNone) は副作用で、コミットメッセージが「Bytes::from_iter のメモリレイアウトが違う」と誤診したため fast path を丸ごと無効化していました。実際は Bytes::from_iter も内部で Bytes::from_array(iter.collect()) を呼ぶだけで表現は同一です。

修正

  • < 55< 56 (one-block padding は remainder ≤ 55 で OK)
  • sha1_raw / sha1_bytes を 1-shot FFI (sha1_compute_ffi) に戻し、per-block FFI を経由する遅いパスを廃止
  • 両方の padding cliff (55 / 119 バイト) を歩く回帰テストを追加

Bench (moon bench -p mizchi/bit_hash --target native --release, scalar fallback CPU)

Benchmark regressed main fixed Δ
sha1_raw 64 bytes 1.18 µs 842 ns −29%
sha1_raw 1 KiB 9.46 µs 6.72 µs −29%
sha1_raw 8 KiB 69.51 µs 50.65 µs −27%
sha1_raw 64 KiB 546.63 µs 403.00 µs −26%

(docs/benchmarks.md 上部の数値は SHA-NI 付き CPU の天井で、この PR 単体ではそこまで届きませんが、scalar fallback の理論限界に乗りました。)

docs/benchmarks.md に修正経緯と moon-pprof で見えた mizchi/simd::simdhash::rotl/rotr の非インライン化 (wasm-gc で self time の 45%) を追記しています。後者は upstream の mizchi/simd への提案で、本 PR では触っていません。

Test plan

  • moon test -p mizchi/bit_hash --target native --release — 15/15 passed (新しい padding cliff sweep test を含む)
  • moon test -p mizchi/bit_hash --target wasm-gc --release — 15/15 passed
  • moon test -p mizchi/bit_object --target native --release — 27/27 passed
  • moon test -p mizchi/bit_lib --target native --release — 285/285 passed
  • moon test -p mizchi/bitx_hub --target native --release — 113/113 passed (c25986d の元々の regression を踏むパス)
  • moon test -p mizchi/bit_pack --target native --release — 72/72 passed
  • node tools/check-layers.mjs — clean
  • CI で cmd/bit の monolithic test が pass することを確認 (この VM では gcc -O2 が 10 分以上かかって local では完走できず)

https://claude.ai/code/session_01FgY6EMujwhzucSQkBVcodR


Generated by Claude Code

The native one-shot `sha1_compute` C entry had `if (remainder < 55)` for
the "padding fits in one block" branch. At `remainder == 55` — exactly
the case where the 0x80 marker plus the 8-byte big-endian length still
fit in the current 64-byte block — the comparison rejected the input
and routed it through the two-block path, producing a different digest
than the incremental `Sha1State` walk. Commit c25986d disabled
`sha1_compute_ffi` entirely as a workaround, masking the bug and
making `sha1_raw` pay one C FFI call per 64-byte block. Fix the
boundary (`< 56`) and restore `sha1_raw` to the one-shot path; add a
sweep test that walks both padding cliffs (55-byte and 119-byte).

`moon bench -p mizchi/bit_hash --target native --release` on a scalar
(non-SHA-NI) CPU:

  sha1_raw 64 bytes   1.18 µs ->  842 ns  (-29%)
  sha1_raw 1 KiB      9.46 µs -> 6.72 µs  (-29%)
  sha1_raw 8 KiB     69.51 µs -> 50.65 µs (-27%)
  sha1_raw 64 KiB   546.63 µs -> 403.00 µs (-26%)

bit_hash, bit_object, bit_lib, bitx_hub, bit_pack tests all pass (15 +
27 + 285 + 113 + 72 = 512). HubStore's `put_record` / `get_record`
round-trip — the original c25986d regression — exercises the fixed
path via `array_to_bytes` -> `sha1`.

https://claude.ai/code/session_01FgY6EMujwhzucSQkBVcodR
@mizchi mizchi merged commit a014165 into main May 31, 2026
16 checks passed
@mizchi mizchi deleted the claude/eloquent-bell-B6IAk branch May 31, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants