perf: read and write a run of adjacent bit fields in one call - #677
perf: read and write a run of adjacent bit fields in one call#677Simon-Calbert-Aerospacelab wants to merge 12 commits into
Conversation
|
One limitation worth flagging, because I think it decides how much this PR is The eligibility rule requires the field type to be #[deku(bits = 1)]
ocf: OcfFlag, // not eligible, splits the run
#[deku(bits = 3)]
version: u8, // eligibleEvery enum-typed field cuts the run in two. On the CCSDS TM primary header I use The reason the derive cannot just accept enums is that when it expands a struct it I have that working on my fork, and it is a smaller change than I expected: The shape is a The one thing I could not make automatic is a single opt-in on the container, I would rather get your read on the design before I open it here, since the opt-in |
|
Two follow-ups while addressing the review.
|
|
@Simon-Calbert-Aerospacelab looks like one of the tests is failing. |
Turned out to be a Also spotted that a batched run wasn't emitting the per-field |
6e004b1 to
0ad31f9
Compare
Follow-up to #673, which is now in master.
What this changes
#673 made a single bit field cheap by keeping the value in a
u64instead of aBitSlice. But a header is not one bit field, it is a dozen of them in a row,and each still costs its own
read_bits_uint_msb0call, its own leftoverbookkeeping, and its own
bits_readupdate.The fields are contiguous on the wire. Nothing between them moves the cursor. So
the derive can read the whole group with one call and cut the individual
fields out with a shift and a mask, both of which are compile-time constants.
What the derive emits
For three adjacent big-endian fields of 2, 10 and 4 bits, the read used to be
three calls. It is now:
The write side composes the same integer in reverse and makes one
write_bits_uint_msb0call.Two details that keep this behaviour-preserving rather than merely faster:
0b1111into a 2-bit fieldused to fail with "bit size of input is larger than requested size". Folding
fields into one integer would have silently let the high bits collide with the
neighbour, so the derive emits a
check_bit_size(value, bits)call per fieldbefore composing. Same error, same message.
current run and starts a new one rather than overflowing.
On a real frame
The CCSDS TM primary header is 11 fields packed into 48 bits. It went from 11
reads and 11 writes to 1 and 1.
When a run is not formed
The planner is deliberately conservative. It requires at least two consecutive
eligible fields, and a field is eligible only if:
the target's endianness, which is little on x86, so absent is rejected rather
than assumed.
Msb0, which is the default, so absent is fine butlsbisnot.
u8/u16/u32/u64, andbits(if present) is a literal in1..=width.bytes,count,bits_read,bytes_read,until,read_all,map,ctx,update,reader,writer,skip, all fourpad_*,temp,temp_value,cond,assert,assert_eq,all four
seek_*, andmagic. Each of those either moves the cursor, makesthe read conditional, or depends on a value read earlier, and any one of them
breaks the "one contiguous read" assumption.
Anything ineligible keeps exactly the code it generates today, and an ineligible
field simply splits the run in two. So this is additive: no existing derive
changes behaviour, it either batches or it does not.
Concretely, per file
deku-derive/src/macros/deku_read.rs:run_field(is this fieldeligible),
plan_bit_runs(group maximal eligible spans, capped at 64 bits),emit_bit_run_read(one read plus constant shifts and masks).deku-derive/src/macros/deku_write.rs: the dual, plus the per-fieldcheck_bit_sizecalls.src/reader.rs/src/writer.rs:read_bits_uint_msb0andwrite_bits_uint_msb0becomepubso the derive can call them, andcheck_bit_sizeis added. No logic change to either.Numbers
cargo bench --bench bebits --all-features, baselined against master at90989ff, so layer 1 is in both arms and this isolates the batching. Dividedout to one header, from benches that push 128 frames through a single
reader/writer:
u8fieldsu8fieldsNote the byte-aligned rows. In #673 those were a control, because that PR only
touched the sub-byte path. Here they are a genuine win: six adjacent big-endian
u8fields are 48 contiguous bits, so they now batch like any other run.The lone 1-bit field is the control for this PR. One field cannot form a run, so
its codegen is byte-for-byte identical; the +0.9% is noise.
Testing
No test file changes, which is the point: the entire existing suite passes
unmodified across all eight CI feature configurations. That is a meaningful
gate here, because this changes codegen for every derived type in every test.