feat(bench): add Criterion.rs benchmark scaffold (mohu-benchmarks) - #248
Open
sonusharma6-dsa wants to merge 25 commits into
Open
feat(bench): add Criterion.rs benchmark scaffold (mohu-benchmarks)#248sonusharma6-dsa wants to merge 25 commits into
sonusharma6-dsa wants to merge 25 commits into
Conversation
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com> (cherry picked from commit 7bbd6db) Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
…ssary casts) Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
…and fft modules Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: GitHub Copilot <noreply@github.com>
Signed-off-by: GitHub Copilot <noreply@github.com>
Signed-off-by: GitHub Copilot <noreply@github.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
📝 WalkthroughWalkthroughAdds a CSV I/O module with tests, FFT frequency utilities and 1D transforms with tests, a new benchmarks crate and workflow, Windows LLVM-MinGW script/docs, workspace tweaks, and extensive formatting-only refactors across multiple crates. ChangesWorkspace-wide updates and features
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Author
|
Thanks for the auto-review, @coderabbitai — useful summary. Next steps from my side:\n\n- I'll add a focused unit test demonstrating bench integration (and a minimal mohu-array usage example) in this branch.\n- Then I'll extend the benches to call mohu-array compute paths (ufuncs + reductions) and run cargo bench locally to capture baseline Criterion reports.\n- After that I'll add a CI job suggestion to run benches on a schedule and upload reports for regression tracking.\n\nIf maintainers prefer, I can open a follow-up draft PR for the mohu-array-specific benches so this initial scaffold remains small.\n\nCc: @mohu-org — can a mentor be assigned to guide the scope and CI integration? |
|
Tip For best results, initiate chat on the files or code changes.
Unit test / mohu-array usage example
Extending benches to mohu-array compute paths
CI job for scheduled bench runs
Draft PR approach
Regarding mentor assignment, that's entirely up to the 🐇 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mohu-buffer/src/view.rs (1)
106-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
// SAFETY:invariants for every undocumentedunsafeblock incrates/mohu-buffer/src/view.rs
- Missing
// SAFETY:comments for theseunsafe { ... }blocks:get_unchecked(lines 117-120),get_mut(lines 209-213),set’sunsafe { ... }(lines 216-221), anditer_mut’sunsafe { ... }(lines 236-238).- The
getblock’sunsafepointer creation at lines 106-108 is already adjacent to a// SAFETY:comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mohu-buffer/src/view.rs` around lines 106 - 109, Add explicit "// SAFETY:" comments for each undocumented unsafe block in get_unchecked, get_mut, set, and iter_mut: state the invariants that make the unsafe operations valid — that self.buf.as_ptr() (or as_mut_ptr()) is non-null and points to a valid allocation for the lifetime, the computed offset (off / idx) is within the buffer bounds/capacity (as guaranteed by previous checks or internal byte_offset logic), the pointer is correctly aligned for T, the memory at that location is properly initialized when read, and for mutable accesses (get_mut, set, iter_mut) there are no aliasing/borrowing violations (exclusive &mut access). Mirror the style of the existing SAFETY comment in get for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 26: Replace the mutable action reference
"christophebedard/dco-check@0.5.1" with an immutable commit SHA: locate the
desired commit in the christophebedard/dco-check repository (e.g., the commit
corresponding to the 0.5.1 release) and update the workflow step to use
"christophebedard/dco-check@<full-commit-sha>" so the CI uses a pinned,
immutable action reference.
In `@crates/mohu-benchmarks/Cargo.toml`:
- Around line 6-10: The crate currently pins criterion locally in
crates/mohu-benchmarks/Cargo.toml; move the criterion version entry into the
root Cargo.toml under [workspace.dependencies] and then update the crate entry
(criterion in crates/mohu-benchmarks) to use the workspace-managed dependency by
using criterion = { workspace = true, features = ["html_reports"] } (or omit
features if they match the root), ensuring the workspace follows the rule that
all external dependency versions live at the root and crates reference them with
workspace = true.
In `@crates/mohu-benchmarks/README.md`:
- Line 12: The README line "I/O benchmarks for `.npy`/.csv` using `mohu-io`" has
mismatched backticks around .csv; edit that string so both extensions are inline
code — replace it with "I/O benchmarks for `.npy`/`.csv` using `mohu-io`"
(update the text in the README.md entry containing that sentence).
In `@crates/mohu-buffer/src/alloc.rs`:
- Around line 442-444: Add a `// SAFETY:` comment immediately above each unsafe
block that uses SIMD intrinsics and raw pointer arithmetic (e.g., the blocks
calling `_mm_prefetch(ptr.add(offset) as *const i8, _MM_HINT_T0)` and the other
similar blocks around the other `_mm_prefetch`/`ptr.add(...)` usages),
describing the invariants that make the unsafety sound: that `ptr` is non-null
and properly aligned for the accessed bytes (or that alignment isn’t required
for this intrinsic), that `offset` is in-bounds for the allocation or pointer
provenance, and that the CPU supports the used intrinsic; ensure each comment is
specific to that block (mention the exact pointer and offset variables used) and
placed immediately above the corresponding `unsafe { ... }` block.
In `@crates/mohu-buffer/src/buffer.rs`:
- Around line 1954-1977: The From<Vec<...>> impls for Buffer (e.g. impl
From<Vec<f64>> for Buffer, impl From<Vec<u8>> for Buffer) call
Buffer::from_vec(...).expect(...) which can panic; remove these panics by making
the conversions fallible: replace the From<Vec<T>> impls with TryFrom<Vec<T>>
implementations that call Buffer::from_vec(v) and propagate the
Result<MohuError, Buffer> (or the crate's standard error type) instead of
unwrapping, and update any callers to handle the Result; ensure you reference
Buffer::from_vec and the specific impl blocks when changing signatures so the
error type and propagation use the crate's MohuError (or existing error alias).
- Around line 551-553: The unsafe block computing data_ptr must be annotated
with a // SAFETY: comment that states the invariants relied on: that self.raw is
a valid, properly aligned pointer for the allocation, that as_mut_ptr() yields a
pointer to a buffer at least layout.offset() bytes long so
.add(self.layout.offset()) does not overflow, that the resulting pointer is
correctly aligned for the intended type and valid for reads/writes for the
lifetime used, and that casting to *mut c_void is safe; apply equivalent SAFETY
comments to the other changed unsafe blocks you touched (the blocks around the
other offsets at the ranges mentioned) and reference the same specific symbols
(self.raw, as_mut_ptr(), self.layout.offset(), and any pointer arithmetic or
casts used) to document the exact invariant for each block.
In `@crates/mohu-buffer/src/layout.rs`:
- Line 470: The computation of new_offset using (self.offset as isize + start as
isize * self.strides[axis]) as usize can wrap on negative signed results; change
this to perform checked signed arithmetic (e.g., compute delta = start as isize
* self.strides[axis], then use self.offset as isize .checked_add(delta) and
ensure the result is >= 0) and return an Err on underflow instead of casting;
update the code that defines new_offset (referencing self.offset, start,
self.strides, axis, and new_offset) to use the checked operations and convert to
usize only after verifying non-negativity.
- Around line 103-107: The reversed-slice count calculation incorrectly treats
the sentinel stop == usize::MAX as a normal value, causing
start.saturating_sub(stop) to yield 0 for omitted stop (so slices like ::-1
become empty); in the block that returns
start.saturating_sub(stop).div_ceil(abs_step) (use e_reversed_empty, start,
stop, abs_step, div_ceil to locate it), special-case the sentinel by using
(start + 1) as the distance when stop == usize::MAX, otherwise use
start.saturating_sub(stop); then apply div_ceil(abs_step) to that distance so
counts for omitted stop are computed correctly.
In `@crates/mohu-buffer/src/ops.rs`:
- Around line 1344-1348: Add block-local "// SAFETY:" comments to the two unsafe
blocks that perform pointer arithmetic and writes (the blocks using
ptr.add(i).write(value) and let aligned_ptr = unsafe { ptr.add(prefix) }); for
each, state the invariants: that `ptr` is non-null and properly aligned for the
pointee type, that `i`/`prefix` are within the allocated bounds so the resulting
pointer is in-bounds, that writing via `.write()` is valid because the target
memory is uninitialized or exclusively owned (no aliasing), and any
lifetime/borrowing guarantees that prevent concurrent access; apply the same
explicit SAFETY comment style to the other unsafe block around lines 1365–1367
as well.
- Around line 974-977: The call to
src.layout().byte_offset(idx.as_slice()).expect(...) in flip_axis_copy can
panic; change it to return a MohuError instead (e.g., use
.byte_offset(...).ok_or_else(|| MohuError::...)? ) so flip_axis_copy returns Err
on failure; update the function signature/propagation to use MohuResult if
necessary and reference flip_axis_copy, src.layout().byte_offset, and the idx
(NdIndexIter) value when constructing the error.
In `@crates/mohu-buffer/src/strides.rs`:
- Around line 108-113: The broadcast_strides function currently zips src_shape
and src_strides which can silently truncate if their lengths differ; add an
explicit check that src_shape.len() == src_strides.len() at the start of
broadcast_strides and return a MohuResult error (with a clear message) if they
differ so we never silently ignore trailing axes; apply the same explicit length
check to the other related function handling broadcasting (the analogous block
at lines ~132-144) to ensure both places validate src_shape vs src_strides
before zipping/iterating.
In `@crates/mohu-buffer/tests/integration.rs`:
- Around line 30-35: The test currently uses a manual epsilon loop
(assert!(buf.as_slice::<f32>().unwrap().iter().all(|&x| (x - 3.14_f32).abs() <
1e-6))) to compare floats; replace that with the project helper macro
mohu_testing::assert_allclose!, calling it with buf.as_slice::<f32>().unwrap()
as the actual, an expected slice of 3.14_f32 repeated the correct length, and
atol = 1e-6; make the same replacement for the other manual f32 comparison
around the buf.as_slice::<f32>() occurrence later in the file.
In `@crates/mohu-error/src/reporter.rs`:
- Around line 231-234: The JSON building in fmt_json is manually escaping
strings (e.g., the ctx_json mapping over ctxs using replace) which fails for
backslashes and control chars; replace those manual escapes by calling
serde_json::to_string for each string field (e.g., when building ctx_json and
the other similar mappings at the noted locations) and use the returned
quoted/escaped string in the JSON output; ensure you handle the Result from
serde_json::to_string (unwrap_or_else or map_err) consistently within fmt_json
so malformed data never produces invalid JSON.
In `@crates/mohu-io/src/csv.rs`:
- Around line 15-47: Replace the local CsvError enum and CsvResult alias with
the workspace-wide MohuError and MohuResult<T>: remove CsvError/CsvResult,
change all public function signatures returning CsvResult<T> to MohuResult<T>
(search for CsvResult, CsvError, ColumnMismatch, EmptyFile, Io, Parse, Utf8),
and map the underlying causes (std::io::Error, csv::Error,
std::string::FromUtf8Error and the ColumnMismatch/EmptyFile semantic errors)
into appropriate MohuError variants (or add central variants in mohu-error if
missing) by converting or wrapping errors where they originate (implement From
conversions or explicit mapping in reader/writer functions). Ensure all uses at
the other noted ranges (around the other occurrences mentioned) are updated to
return MohuResult and use MohuError instead of the local CsvError.
- Around line 243-267: The ColumnMismatch error currently uses record_index + 1,
which counts skipped rows; change the reported row to be relative to the "data
section" by computing a data_row =
record_index.saturating_sub(self.opts.skip_rows) + 1 (or equivalent checked
subtraction) and pass that data_row into CsvError::ColumnMismatch::row in the
match arm; update the code around the for loop/match where
CsvError::ColumnMismatch is constructed (referencing records.enumerate(),
record_index, self.opts.skip_rows, and CsvError::ColumnMismatch) so the error
reports the row number within the data section, not including skipped rows.
In `@crates/mohu-io/src/lib.rs`:
- Around line 6-9: Add a crate-root rustdoc block above the pub use that
documents the CSV re-exports (CsvError, CsvReader, CsvResult, CsvTable,
CsvValue, CsvWriter, ReadOptions, WriteOptions, read_csv, write_csv) and include
a `# Example` section showing minimal usage (e.g., creating a CsvWriter or
calling write_csv to write rows and using read_csv or CsvReader to read them
back), so the public CSV API has at least one runnable example; place the doc
comment directly above the pub use in lib.rs and ensure the example compiles
(include use statements and any small sample data) to satisfy the public-API
documentation guideline.
In `@scripts/use-llvm-mingw.ps1`:
- Around line 39-40: The two diagnostic lookups using Get-Command for
dlltool.exe and gcc.exe should not hard-fail the script; update the lookups
(refs: the Write-Host lines that call (Get-Command dlltool.exe).Source and
(Get-Command gcc.exe).Source) to call Get-Command with -ErrorAction
SilentlyContinue (or capture the result into a variable), then conditionally
Write-Host only if the command object is non-null so the script continues even
if gcc.exe is missing (linker is rust-lld.exe).
---
Outside diff comments:
In `@crates/mohu-buffer/src/view.rs`:
- Around line 106-109: Add explicit "// SAFETY:" comments for each undocumented
unsafe block in get_unchecked, get_mut, set, and iter_mut: state the invariants
that make the unsafe operations valid — that self.buf.as_ptr() (or as_mut_ptr())
is non-null and points to a valid allocation for the lifetime, the computed
offset (off / idx) is within the buffer bounds/capacity (as guaranteed by
previous checks or internal byte_offset logic), the pointer is correctly aligned
for T, the memory at that location is properly initialized when read, and for
mutable accesses (get_mut, set, iter_mut) there are no aliasing/borrowing
violations (exclusive &mut access). Mirror the style of the existing SAFETY
comment in get for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2cdbda32-babd-41e2-87f9-c5bc68847f26
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.github/workflows/ci.ymlCargo.tomlREADME.mdcrates/mohu-array/src/array.rscrates/mohu-array/src/iter.rscrates/mohu-array/src/shape.rscrates/mohu-array/src/slice.rscrates/mohu-array/src/view.rscrates/mohu-benchmarks/Cargo.tomlcrates/mohu-benchmarks/README.mdcrates/mohu-benchmarks/benches/bench_core.rscrates/mohu-buffer/examples/alloc_and_pool.rscrates/mohu-buffer/examples/buffer_basics.rscrates/mohu-buffer/src/alloc.rscrates/mohu-buffer/src/buffer.rscrates/mohu-buffer/src/layout.rscrates/mohu-buffer/src/lib.rscrates/mohu-buffer/src/ops.rscrates/mohu-buffer/src/pool.rscrates/mohu-buffer/src/strides.rscrates/mohu-buffer/src/view.rscrates/mohu-buffer/tests/integration.rscrates/mohu-dtype/examples/dtype_basics.rscrates/mohu-dtype/examples/type_promotion.rscrates/mohu-dtype/src/cast.rscrates/mohu-dtype/src/compat.rscrates/mohu-dtype/src/dlpack.rscrates/mohu-dtype/src/dtype.rscrates/mohu-dtype/src/finfo.rscrates/mohu-dtype/src/iinfo.rscrates/mohu-dtype/src/lib.rscrates/mohu-dtype/src/macros.rscrates/mohu-dtype/src/promote.rscrates/mohu-dtype/src/scalar.rscrates/mohu-error/src/codes.rscrates/mohu-error/src/context.rscrates/mohu-error/src/error.rscrates/mohu-error/src/kind.rscrates/mohu-error/src/macros.rscrates/mohu-error/src/multi.rscrates/mohu-error/src/python.rscrates/mohu-error/src/reporter.rscrates/mohu-error/src/test_utils.rscrates/mohu-fft/Cargo.tomlcrates/mohu-fft/src/freq helpers nd norm plan real transform.rscrates/mohu-fft/src/freq.rscrates/mohu-fft/src/lib.rscrates/mohu-fft/src/transform.rscrates/mohu-index/src/boolean fancy gather slice take where_op.rscrates/mohu-index/src/lib.rscrates/mohu-io/Cargo.tomlcrates/mohu-io/src/arrow.rscrates/mohu-io/src/csv.rscrates/mohu-io/src/lib.rscrates/mohu-io/src/mmap.rscrates/mohu-io/src/npy.rscrates/mohu-io/tests/csv_tests.rscrates/mohu-masked/src/arith array compress fill io mask_ops reduce.rscrates/mohu-masked/src/lib.rscrates/mohu-ops/src/arith.rscrates/mohu-ops/src/broadcast.rscrates/mohu-ops/src/cmp.rscrates/mohu-ops/src/logical.rscrates/mohu-ops/src/reduce.rscrates/mohu-ops/src/unary.rscrates/mohu-random/src/continuous discrete entropy generator multivariate permutation seeding.rscrates/mohu-random/src/generator.rscrates/mohu-random/src/lib.rscrates/mohu-simd/src/arith bitwise cast cmp copy fill fma math reduce detect.rscrates/mohu-simd/src/lib.rscrates/mohu-sparse/src/arith bsr coo csc csr convert dia linalg slice spmm spmv.rscrates/mohu-sparse/src/lib.rscrates/mohu-special/src/beta bessel erf expint gamma misc stats_fn trig.rscrates/mohu-special/src/lib.rscrates/mohu-stats/src/descriptive.rscrates/mohu-stats/src/distributions.rscrates/mohu-stats/src/random.rscrates/mohu-stats/src/sampling.rscrates/mohu-testing/src/approx assert dtype fixtures gen perf.rscrates/mohu-testing/src/assert.rscrates/mohu-testing/src/lib.rscrates/mohu-ufunc/src/broadcast dispatch loop_impl macros methods reduce resolver traits.rscrates/mohu-ufunc/src/lib.rsscripts/README.mdscripts/use-llvm-mingw.ps1
💤 Files with no reviewable changes (15)
- crates/mohu-random/src/continuous
discrete
entropy
generator
multivariate
permutation
seeding.rs - crates/mohu-special/src/beta
bessel
erf
expint
gamma
misc
stats_fn
trig.rs - crates/mohu-fft/src/freq
helpers
nd
norm
plan
real
transform.rs - crates/mohu-simd/src/arith
bitwise
cast
cmp
copy
fill
fma
math
reduce
detect.rs - crates/mohu-masked/src/arith
array
compress
fill
io
mask_ops
reduce.rs - crates/mohu-testing/src/approx
assert
dtype
fixtures
gen
perf.rs - crates/mohu-ufunc/src/broadcast
dispatch
loop_impl
macros
methods
reduce
resolver
traits.rs - crates/mohu-sparse/src/arith
bsr
coo
csc
csr
convert
dia
linalg
slice
spmm
spmv.rs - crates/mohu-index/src/lib.rs
- crates/mohu-random/src/lib.rs
- crates/mohu-error/src/context.rs
- crates/mohu-masked/src/lib.rs
- crates/mohu-simd/src/lib.rs
- crates/mohu-index/src/boolean
fancy
gather
slice
take
where_op.rs - crates/mohu-ufunc/src/lib.rs
| if: github.event_name == 'pull_request' | ||
| steps: | ||
| - uses: christophebedard/dco-check@v0.5.0 | ||
| - uses: christophebedard/dco-check@0.5.1 |
There was a problem hiding this comment.
Pin the GitHub Action to a full commit SHA, not a tag.
Using @0.5.1 is still mutable and can be retagged. Pin this action to an immutable commit SHA to prevent supply-chain drift in CI.
Suggested change
- - uses: christophebedard/dco-check@0.5.1
+ - uses: christophebedard/dco-check@<full_commit_sha>🧰 Tools
🪛 zizmor (1.25.2)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 26, Replace the mutable action reference
"christophebedard/dco-check@0.5.1" with an immutable commit SHA: locate the
desired commit in the christophebedard/dco-check repository (e.g., the commit
corresponding to the 0.5.1 release) and update the workflow step to use
"christophebedard/dco-check@<full-commit-sha>" so the CI uses a pinned,
immutable action reference.
Comment on lines
+6
to
+10
| [dependencies] | ||
| criterion = { version = "0.4", features = ["html_reports"] } | ||
| # Use the workspace mohu-array crate when available; maintainers can enable this | ||
| # once the public API for benchmarks is agreed. | ||
| mohu-array = { workspace = true } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether criterion is centralized in root workspace deps
rg -n --glob 'Cargo.toml' '^\[workspace\.dependencies\]|^criterion\s*=' -C2Repository: mohu-org/mohu
Length of output: 747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show root workspace dependency block lines around where criterion would be declared
root_cargo="Cargo.toml"
echo "== Root Cargo.toml: workspace.dependencies (lines 40-120) =="
nl -ba "$root_cargo" | sed -n '40,120p'
echo "== Search root Cargo.toml for criterion declaration =="
rg -n '^\s*criterion\s*=' "$root_cargo" || true
echo "== Search entire repo for criterion entries in [workspace.dependencies] =="
rg -n --glob 'Cargo.toml' --context 2 '^\[workspace\.dependencies\]|\bcriterion\s*=\s*' "$root_cargo" crates || trueRepository: mohu-org/mohu
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root_cargo="Cargo.toml"
echo "== Find [workspace.dependencies] range in root Cargo.toml =="
grep -n '^\[workspace\.dependencies\]' -n "$root_cargo" || true
echo "== Search root Cargo.toml for criterion declarations =="
rg -n '^\s*criterion\s*=' "$root_cargo" || true
echo "== Print workspace.dependencies block (approx. lines 35-110) =="
sed -n '35,110p' "$root_cargo"Repository: mohu-org/mohu
Length of output: 3748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob 'Cargo.toml' 'criterion\s*=\s*\{' .
rg -n --glob 'Cargo.toml' '^\s*criterion\s*=' .
rg -n --glob 'Cargo.toml' '\[workspace\.dependencies\]' Cargo.tomlRepository: mohu-org/mohu
Length of output: 283
Use workspace-managed dependency for criterion (move it to root [workspace.dependencies])
crates/mohu-benchmarks/Cargo.toml pins criterion locally:
[dependencies]
criterion = { version = "0.4", features = ["html_reports"] }This conflicts with the workspace rule (“all external dependency versions must live in root [workspace.dependencies] and crates must use { workspace = true }). Add criterion to the root [workspace.dependencies], then change the crate entry to use criterion = { workspace = true, features = ["html_reports"] } (or omit features if identical to root).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-benchmarks/Cargo.toml` around lines 6 - 10, The crate currently
pins criterion locally in crates/mohu-benchmarks/Cargo.toml; move the criterion
version entry into the root Cargo.toml under [workspace.dependencies] and then
update the crate entry (criterion in crates/mohu-benchmarks) to use the
workspace-managed dependency by using criterion = { workspace = true, features =
["html_reports"] } (or omit features if they match the root), ensuring the
workspace follows the rule that all external dependency versions live at the
root and crates reference them with workspace = true.
Comment on lines
+442
to
+444
| unsafe { | ||
| _mm_prefetch(ptr.add(offset) as *const i8, _MM_HINT_T0); | ||
| } |
There was a problem hiding this comment.
Add // SAFETY: comments to each unsafe block.
These unsafe blocks are missing invariant documentation in-place. Please add // SAFETY: comments immediately above each block explaining why the operation is sound.
As per coding guidelines **/*.rs: Every unsafe block needs a // SAFETY: comment documenting the invariant.
Also applies to: 488-490, 623-625, 641-643
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-buffer/src/alloc.rs` around lines 442 - 444, Add a `// SAFETY:`
comment immediately above each unsafe block that uses SIMD intrinsics and raw
pointer arithmetic (e.g., the blocks calling `_mm_prefetch(ptr.add(offset) as
*const i8, _MM_HINT_T0)` and the other similar blocks around the other
`_mm_prefetch`/`ptr.add(...)` usages), describing the invariants that make the
unsafety sound: that `ptr` is non-null and properly aligned for the accessed
bytes (or that alignment isn’t required for this intrinsic), that `offset` is
in-bounds for the allocation or pointer provenance, and that the CPU supports
the used intrinsic; ensure each comment is specific to that block (mention the
exact pointer and offset variables used) and placed immediately above the
corresponding `unsafe { ... }` block.
Comment on lines
+551
to
553
| let data_ptr = | ||
| unsafe { self.raw.as_mut_ptr().add(self.layout.offset()) as *mut std::ffi::c_void }; | ||
|
|
There was a problem hiding this comment.
Document safety invariants for changed unsafe blocks.
Several changed unsafe blocks are missing // SAFETY: comments. Please annotate each block with the exact invariant being relied on.
As per coding guidelines **/*.rs: Every unsafe block needs a // SAFETY: comment documenting the invariant.
Also applies to: 1001-1002, 1039-1040, 1849-1853, 1885-1889, 1894-1898
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-buffer/src/buffer.rs` around lines 551 - 553, The unsafe block
computing data_ptr must be annotated with a // SAFETY: comment that states the
invariants relied on: that self.raw is a valid, properly aligned pointer for the
allocation, that as_mut_ptr() yields a pointer to a buffer at least
layout.offset() bytes long so .add(self.layout.offset()) does not overflow, that
the resulting pointer is correctly aligned for the intended type and valid for
reads/writes for the lifetime used, and that casting to *mut c_void is safe;
apply equivalent SAFETY comments to the other changed unsafe blocks you touched
(the blocks around the other offsets at the ranges mentioned) and reference the
same specific symbols (self.raw, as_mut_ptr(), self.layout.offset(), and any
pointer arithmetic or casts used) to document the exact invariant for each
block.
Comment on lines
+231
to
234
| let ctx_json: Vec<String> = ctxs | ||
| .iter() | ||
| .map(|s| format!("\"{}\"", s.replace('"', "\\\""))) | ||
| .collect(); |
There was a problem hiding this comment.
Use robust JSON escaping in fmt_json to avoid malformed output.
replace('"', "\\\"") does not escape backslashes or control characters, so valid error/context text can produce invalid JSON. Serialize with a JSON encoder (e.g., serde_json::to_string) for each string field.
Proposed fix
- let ctx_json: Vec<String> = ctxs
- .iter()
- .map(|s| format!("\"{}\"", s.replace('"', "\\\"")))
- .collect();
+ let ctx_json: Vec<String> = ctxs
+ .iter()
+ .map(|s| serde_json::to_string(s).unwrap_or_else(|_| "\"<json-escape-error>\"".to_string()))
+ .collect();
@@
- let primary = lines.next().unwrap_or("").replace('"', "\\\"");
+ let primary = serde_json::to_string(lines.next().unwrap_or(""))
+ .unwrap_or_else(|_| "\"<json-escape-error>\"".to_string());
let hints: Vec<String> = lines
.filter_map(|l| l.strip_prefix("hint: "))
- .map(|h| format!("\"{}\"", h.replace('"', "\\\"")))
+ .map(|h| serde_json::to_string(h).unwrap_or_else(|_| "\"<json-escape-error>\"".to_string()))
.collect();
@@
- r#"{{"code":{code},"kind":"{kind}","message":"{primary}","context":[{ctx}],"hints":[{hints}],"chain_depth":{depth}}}"#,
+ r#"{{"code":{code},"kind":"{kind}","message":{primary},"context":[{ctx}],"hints":[{hints}],"chain_depth":{depth}}}"#,Also applies to: 240-244, 248-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-error/src/reporter.rs` around lines 231 - 234, The JSON building
in fmt_json is manually escaping strings (e.g., the ctx_json mapping over ctxs
using replace) which fails for backslashes and control chars; replace those
manual escapes by calling serde_json::to_string for each string field (e.g.,
when building ctx_json and the other similar mappings at the noted locations)
and use the returned quoted/escaped string in the JSON output; ensure you handle
the Result from serde_json::to_string (unwrap_or_else or map_err) consistently
within fmt_json so malformed data never produces invalid JSON.
Comment on lines
+15
to
+47
| /// Errors produced while reading or writing CSV data. | ||
| #[derive(Debug, Error)] | ||
| pub enum CsvError { | ||
| /// Underlying I/O failure. | ||
| #[error("I/O error: {0}")] | ||
| Io(#[from] std::io::Error), | ||
|
|
||
| /// Failure reported by the `csv` crate. | ||
| #[error("CSV parse error: {0}")] | ||
| Parse(#[from] csv::Error), | ||
|
|
||
| /// UTF-8 conversion failed while producing a `String` output. | ||
| #[error("UTF-8 conversion error: {0}")] | ||
| Utf8(#[from] std::string::FromUtf8Error), | ||
|
|
||
| /// No header or data rows were found. | ||
| #[error("empty file: no records found")] | ||
| EmptyFile, | ||
|
|
||
| /// A data row had a different number of columns than expected. | ||
| #[error("row {row} has {got} columns, expected {expected}")] | ||
| ColumnMismatch { | ||
| /// 1-based row index within the data section. | ||
| row: usize, | ||
| /// Expected number of columns. | ||
| expected: usize, | ||
| /// Actual number of columns. | ||
| got: usize, | ||
| }, | ||
| } | ||
|
|
||
| /// Result type used by CSV reader and writer operations. | ||
| pub type CsvResult<T> = Result<T, CsvError>; |
There was a problem hiding this comment.
Unify CSV APIs to MohuError/MohuResult per workspace contract.
CsvError/CsvResult creates a second error model in mohu-io (Line 15 onward), while public functions return that type. Please switch these APIs to MohuError + MohuResult<T> and map CSV/IO failures into existing MohuError variants (or add variants centrally in mohu-error if needed).
As per coding guidelines, "Use MohuError from mohu-error everywhere for error handling — never anyhow, never Box<dyn Error>" and "Use MohuError and MohuResult<T> instead of anyhow or Box<dyn Error>."
Also applies to: 194-201, 338-347, 409-415
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-io/src/csv.rs` around lines 15 - 47, Replace the local CsvError
enum and CsvResult alias with the workspace-wide MohuError and MohuResult<T>:
remove CsvError/CsvResult, change all public function signatures returning
CsvResult<T> to MohuResult<T> (search for CsvResult, CsvError, ColumnMismatch,
EmptyFile, Io, Parse, Utf8), and map the underlying causes (std::io::Error,
csv::Error, std::string::FromUtf8Error and the ColumnMismatch/EmptyFile semantic
errors) into appropriate MohuError variants (or add central variants in
mohu-error if missing) by converting or wrapping errors where they originate
(implement From conversions or explicit mapping in reader/writer functions).
Ensure all uses at the other noted ranges (around the other occurrences
mentioned) are updated to return MohuResult and use MohuError instead of the
local CsvError.
Comment on lines
+243
to
+267
| for (record_index, result) in records.enumerate() { | ||
| if record_index < self.opts.skip_rows { | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(limit) = self.opts.max_rows { | ||
| if data.len() >= limit { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| let record = result?; | ||
| let cells = record | ||
| .iter() | ||
| .map(|field| CsvValue::infer(field, &missing_values)) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| match ncols { | ||
| None => ncols = Some(cells.len()), | ||
| Some(expected) if cells.len() != expected => { | ||
| return Err(CsvError::ColumnMismatch { | ||
| row: record_index + 1, | ||
| expected, | ||
| got: cells.len(), | ||
| }); |
There was a problem hiding this comment.
ColumnMismatch.row is off when skip_rows > 0.
Line 264 reports record_index + 1, but record_index includes skipped rows. This conflicts with the documented “data section” row numbering and can mislead debugging.
Suggested fix
- for (record_index, result) in records.enumerate() {
+ for (record_index, result) in records.enumerate() {
if record_index < self.opts.skip_rows {
continue;
}
+ let data_row_index = record_index - self.opts.skip_rows;
...
return Err(CsvError::ColumnMismatch {
- row: record_index + 1,
+ row: data_row_index + 1,
expected,
got: cells.len(),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (record_index, result) in records.enumerate() { | |
| if record_index < self.opts.skip_rows { | |
| continue; | |
| } | |
| if let Some(limit) = self.opts.max_rows { | |
| if data.len() >= limit { | |
| break; | |
| } | |
| } | |
| let record = result?; | |
| let cells = record | |
| .iter() | |
| .map(|field| CsvValue::infer(field, &missing_values)) | |
| .collect::<Vec<_>>(); | |
| match ncols { | |
| None => ncols = Some(cells.len()), | |
| Some(expected) if cells.len() != expected => { | |
| return Err(CsvError::ColumnMismatch { | |
| row: record_index + 1, | |
| expected, | |
| got: cells.len(), | |
| }); | |
| for (record_index, result) in records.enumerate() { | |
| if record_index < self.opts.skip_rows { | |
| continue; | |
| } | |
| let data_row_index = record_index - self.opts.skip_rows; | |
| if let Some(limit) = self.opts.max_rows { | |
| if data.len() >= limit { | |
| break; | |
| } | |
| } | |
| let record = result?; | |
| let cells = record | |
| .iter() | |
| .map(|field| CsvValue::infer(field, &missing_values)) | |
| .collect::<Vec<_>>(); | |
| match ncols { | |
| None => ncols = Some(cells.len()), | |
| Some(expected) if cells.len() != expected => { | |
| return Err(CsvError::ColumnMismatch { | |
| row: data_row_index + 1, | |
| expected, | |
| got: cells.len(), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-io/src/csv.rs` around lines 243 - 267, The ColumnMismatch error
currently uses record_index + 1, which counts skipped rows; change the reported
row to be relative to the "data section" by computing a data_row =
record_index.saturating_sub(self.opts.skip_rows) + 1 (or equivalent checked
subtraction) and pass that data_row into CsvError::ColumnMismatch::row in the
match arm; update the code around the for loop/match where
CsvError::ColumnMismatch is constructed (referencing records.enumerate(),
record_index, self.opts.skip_rows, and CsvError::ColumnMismatch) so the error
reports the row number within the data section, not including skipped rows.
Comment on lines
+6
to
+9
| pub use csv::{ | ||
| CsvError, CsvReader, CsvResult, CsvTable, CsvValue, CsvWriter, ReadOptions, WriteOptions, | ||
| read_csv, write_csv, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document new crate-root CSV exports with an example block.
The new public re-exports are added without crate-root docs/examples. Please add rustdoc (with # Example) for the CSV surface exposed here.
As per coding guidelines, "Public API items need doc comments with at least one # Example block."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mohu-io/src/lib.rs` around lines 6 - 9, Add a crate-root rustdoc block
above the pub use that documents the CSV re-exports (CsvError, CsvReader,
CsvResult, CsvTable, CsvValue, CsvWriter, ReadOptions, WriteOptions, read_csv,
write_csv) and include a `# Example` section showing minimal usage (e.g.,
creating a CsvWriter or calling write_csv to write rows and using read_csv or
CsvReader to read them back), so the public CSV API has at least one runnable
example; place the doc comment directly above the pub use in lib.rs and ensure
the example compiles (include use statements and any small sample data) to
satisfy the public-API documentation guideline.
Comment on lines
+39
to
+40
| Write-Host "dlltool => $((Get-Command dlltool.exe).Source)" | ||
| Write-Host "gcc => $((Get-Command gcc.exe).Source)" |
There was a problem hiding this comment.
Avoid hard-failing on diagnostic tool lookup.
Line 40 can abort the script when gcc.exe is absent, even though the linker is explicitly set to rust-lld.exe. Make these lookups non-fatal (or conditionally print when found) so the command still runs.
Proposed fix
-Write-Host "dlltool => $((Get-Command dlltool.exe).Source)"
-Write-Host "gcc => $((Get-Command gcc.exe).Source)"
+$dlltoolCmd = Get-Command dlltool.exe -ErrorAction SilentlyContinue
+$gccCmd = Get-Command gcc.exe -ErrorAction SilentlyContinue
+if ($dlltoolCmd) { Write-Host "dlltool => $($dlltoolCmd.Source)" }
+if ($gccCmd) { Write-Host "gcc => $($gccCmd.Source)" }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Write-Host "dlltool => $((Get-Command dlltool.exe).Source)" | |
| Write-Host "gcc => $((Get-Command gcc.exe).Source)" | |
| $dlltoolCmd = Get-Command dlltool.exe -ErrorAction SilentlyContinue | |
| $gccCmd = Get-Command gcc.exe -ErrorAction SilentlyContinue | |
| if ($dlltoolCmd) { Write-Host "dlltool => $($dlltoolCmd.Source)" } | |
| if ($gccCmd) { Write-Host "gcc => $($gccCmd.Source)" } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/use-llvm-mingw.ps1` around lines 39 - 40, The two diagnostic lookups
using Get-Command for dlltool.exe and gcc.exe should not hard-fail the script;
update the lookups (refs: the Write-Host lines that call (Get-Command
dlltool.exe).Source and (Get-Command gcc.exe).Source) to call Get-Command with
-ErrorAction SilentlyContinue (or capture the result into a variable), then
conditionally Write-Host only if the command object is non-null so the script
continues even if gcc.exe is missing (linker is rust-lld.exe).
Signed-off-by: sonusharma6-dsa <ramprashadkumar11@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/benchmark.yml (1)
18-20: 💤 Low valueConsider pinning actions to commit SHAs for supply-chain security.
Using tag references (e.g.,
@v4,@stable) instead of commit hashes makes workflows vulnerable to tag manipulation. Pinning to SHAs is a security best practice.🔒 Example of pinned references
- - uses: actions/checkout@v4 + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - - uses: dtolnay/rust-toolchain@stable + - uses: dtolnay/rust-toolchain@00b49be78f40fba4e87296b2ead62868750bdd83 # stable - - uses: Swatinem/rust-cache@v2 + - uses: Swatinem/rust-cache@23bce251a8cd2ffc3c1075eaa2367cf899916d84 # v2.7.3Apply the same pattern to the upload-artifact actions on lines 26 and 32.
As per static analysis hints from zizmor: action is not pinned to a hash (required by blanket policy).
Also applies to: 26-26, 32-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/benchmark.yml around lines 18 - 20, Replace loose action tags with pinned commit SHAs to satisfy supply-chain security: update the uses entries for actions/checkout@v4, dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2 and both upload-artifact uses to their corresponding commit SHA pins (e.g., actions/checkout@<sha>) so the workflow references immutable commits; locate these strings in the workflow and swap the tag portion (e.g., `@v4`, `@stable`, `@v2`) with the correct commit hash for each action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/benchmark.yml:
- Line 35: The artifact path "crates/mohu-benchmarks/target/criterion/" is
incorrect because cargo bench run from the crate writes results to the
workspace's target directory; update the path in .github/workflows/benchmark.yml
to point at the workspace target (for example replace
"crates/mohu-benchmarks/target/criterion/" with "target/criterion/" or
"crates/mohu-benchmarks/../target/criterion/") so the Criterion reports are
uploaded from the actual output directory.
---
Nitpick comments:
In @.github/workflows/benchmark.yml:
- Around line 18-20: Replace loose action tags with pinned commit SHAs to
satisfy supply-chain security: update the uses entries for actions/checkout@v4,
dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2 and both upload-artifact
uses to their corresponding commit SHA pins (e.g., actions/checkout@<sha>) so
the workflow references immutable commits; locate these strings in the workflow
and swap the tag portion (e.g., `@v4`, `@stable`, `@v2`) with the correct commit hash
for each action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8747cab-6bb0-4e0a-ad68-c108ce7fbbbf
📒 Files selected for processing (3)
.github/workflows/benchmark.ymlcrates/mohu-benchmarks/README.mdcrates/mohu-benchmarks/tests/integration_test.rs
✅ Files skipped from review due to trivial changes (1)
- crates/mohu-benchmarks/tests/integration_test.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new workspace member crates/mohu-benchmarks with a Criterion.rs-based benchmark scaffold (arithmetic, reduction, and stride-access microbenchmarks).\n\nThis is a lightweight starting point; maintainers can extend these benches to use mohu-array, I/O benchmarks, and CI integration for regression detection. See crates/mohu-benchmarks/README.md for details and run instructions.\n\nCC: @mohu-org — requesting mentorship/assignment to extend these to full coverage as outlined in issue #226.
Summary by CodeRabbit
New Features
Documentation
Tests
assert_allclose!macro for floating-point comparisons