Re-organising and cleaning up hist.rs#279
Re-organising and cleaning up hist.rs#279cruzzil wants to merge 10 commits intotrifectatechfoundation:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
lib/compress/hist.rs
Outdated
| // Split workspace into 4 counting tables of 256 u32 each | ||
| let (Counting1, remainder) = workSpace.split_at_mut(256); | ||
| let (Counting2, remainder) = remainder.split_at_mut(256); | ||
| let (Counting3, Counting4) = remainder.split_at_mut(256); |
There was a problem hiding this comment.
something like this can work here, keeps the size in the type
let ([Counting1, Counting2, Counting3, Counting4], &[]) = workSpace.as_chunks::<256>() else {
unreachable!();
};
There was a problem hiding this comment.
Didn't know about this one, great suggestion. Done
There was a problem hiding this comment.
Hmm getting a test failure on this, likely because the workSpace is significantly larger than the requirement. Will investigate.
There was a problem hiding this comment.
Perhaps instead of a slice, I could use a fixed array size for workSpace since it only really needs to be 1024 u32's long, everything else is ignored.
lib/compress/zstd_compress.rs
Outdated
| let mut cSymbolTypeSizeEstimateInBits = 0; | ||
| let mut max = maxCode; | ||
|
|
||
| //SAFETY: workspace must be 4 bytes aligned and >= HIST_WKSP_SIZE in bytes |
There was a problem hiding this comment.
You must justify the unsafety here, not list the requirements. So why is it properly aligned, and why is it of the required size?
There was a problem hiding this comment.
These are pre-existing requirements of the C code. Maybe SAFETY should be changed to NOTE
There was a problem hiding this comment.
I clarified with a note that this was an existing requirement of the C implementation.
| sourceSize: size_t, | ||
| check: HIST_checkInput_e, | ||
| workSpace: *mut u32, | ||
| workSpace: &mut [u32], |
There was a problem hiding this comment.
general note
- is this code well-tested?
- how can this code be benchmarked
you're introducing bounds checks, and while those are mostly harmless, sometimes they cause serious performance regressions. At least on my machine the rust and C code are currently about the same in terms of performance (no significant differences), and I'd like to not regress that.
There was a problem hiding this comment.
Is there a benchmark script that I can run to compare commit performance?
| ip = ip.add(1); | ||
| let fresh1 = &mut (*count.offset(*fresh0 as isize)); | ||
| *fresh1 = (*fresh1).wrapping_add(1); | ||
| pub unsafe fn HIST_add(count: &mut [c_uint], src: *const c_void, srcSize: size_t) { |
There was a problem hiding this comment.
we can move this to zstd_preSplit.rs, it's not used here at all. Also generally it's nicer to create the slice at the call site if that means you can make this function safe.
There was a problem hiding this comment.
Yes probably sits better in zstd_preSplit.rs but kept here just for consistency vs C code whilst the cleanup is being done. I can leave a TODO if that helps.
| for s in 0..256 { | ||
| Counting1[s] += Counting2[s] + Counting3[s] + Counting4[s]; | ||
| if (Counting1[s] > max) { | ||
| max = Counting1[s]; |
There was a problem hiding this comment.
if this code is hot at all we can probably tweak this a bit to get better assembly.
2 participants
Should review the commits in order.