perf: zero-extend big-endian bit fields with load_be - #666
Conversation
|
I bet we can do something like this for LE+LSB. |
|
Good instinct, and I tried it before replying. It does not carry over, and the This PR fixes a padding problem. With big-endian + Msb0 the bits arrive in Lsb0 is not a padding problem. It needs a byte reordering (the existing code I also benchmarked where the pad loop actually bites, by reading a 1-bit field
Little-endian is flat, which matches the source: the pad loop appends with The quadrant that does still have the #665 defect is BE+Lsb0: 27x above, and I would hold off on that quadrant anyway: #657's Happy to take BE+Lsb0 as a follow-up once #657 lands, with the same |
|
Follow-up worth flagging: I applied #657 on top of this branch, and it removes The reason is that the expensive loop lives in the generic branch, the one Measured on top of this PR:
The two patches apply cleanly together and pass So BE+Lsb0 might need no performance patch of its own, and these two can merge |
|
@wcampbell0x2a FYI I have two follow-up PRs sitting on my fork that take the
Numbers below are per-item cost, measured on benches that push 128 frames
Worth noting this PR is the only one of the three that fixes an asymptotic Is there anything you would like changed here before it can go in? And do you |
|
@Simon-Calbert-Aerospacelab I can't give you a time-table of merging. I am only the co-maintainer. |
|
Follow-up to this is now open: #673. Same quadrant, one level up, it skips |
Closes #665.
Reading a
#[deku(bits = N)]field zero-extends it to the container width with:BoundedBitVec::insertshifts the whole backing array right by one bit per call,bit by bit, because the
split_at_mutalias defeats bitvec's word-at-a-timeshift_right. That makes a fieldO(container_bits * (container_bits - N))bitoperations: a 1-bit field in a
u64pays 63 whole-array shifts, about 2 us.BitField::load_bedoes the same zero-extension in one step, and this filealready uses it in the
Lsb0branch below.Change
Two hunks in
src/impls/primitive.rs, one perDekuRead::readimpl(
(Endian, BitSize, Order)and(Endian, BitSize)):f32andf64are notfunty::Integral, so the value goes through the unsigned$innerandfrom_be_bytes, as the existing padded-array path does.bit_size > 0leavesbits = 0panicking ininsertas before.Adds
benches/bebits.rs, since the existingDekuBitsbench declares no endianand never reaches this path.
Scope
Explicit
endian = "big"with the defaultMsb0only. Untouched: noendianattribute (which defaults to target endianness, so little-endian on x86),
endian = "little", and anybit_order = "lsb".I left the little-endian side alone on purpose. #658 is an open correctness bug in
the
Lsb0 + Endian::Littlebranch, so mixing a performance change into it seemedunwise. It is fixable the same way, building
ceil(N/8)bytes with the finalpartial byte right-aligned and combining little-endian, once the intended
semantics are settled. Happy to follow up.
Testing
The full CI feature matrix passes and all nine examples run.
tests/bit_order.rsand
tests/test_lsb_le.rsstay green.A differential sweep of the old conversion against the new one, over
u8,u16,u32andu64, every valid width and 40k random inputs: 4.8M cases, 0mismatches.
Two jobs are already red on unpatched
master, so they are not from this PR:test_compile: the trybuild expected stderr does not match newer rustc.Verified identical by reverting only
primitive.rs.cargo clippy -- -D warnings:src/lib.rs:983callsshift_right, deprecatedin bitvec 1.1.1, which the
"1.0.1"requirement now resolves to. Can fixseparately if you want.
Performance
cargo bench --all-features --bench bebits, against amasterbaseline:be_tm_primary_header_11_fieldsbe_one_bit_in_u64be_six_bytes_aligned(byte-aligned control)--bench dekuis unchanged:deku_read_bits740 to 729 ns (p = 0.34). It didreport +2.2% on
deku_read_byte, but the same binary run twice moves that one-3.0%, so run-to-run drift on an 8 ns benchmark is larger than the effect.
rustc 1.96.0, opt-level 3, no LTO, AMD EPYC 9374F.