sharksforarms / deku

Declarative binary reading and writing: bit-level, symmetric, serialization/deserialization
Apache License 2.0
1.05k stars 54 forks source link

DRAFT: Add aligned attribute #295

Closed wcampbell0x2a closed 10 months ago

wcampbell0x2a commented 1 year ago

This is a draft for now, but I tested out adding an aligned attribute to test the performance increase.

deku_read_vec           time:   [765.59 ns 769.06 ns 772.51 ns]
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe
deku_read_vec_perf      time:   [686.51 ns 691.37 ns 696.95 ns]
Found 8 outliers among 100 measurements (8.00%)
  4 (4.00%) high mild
  4 (4.00%) high severe

not bad

@sharksforarms I'm curious of what you think of the approach of adding an attribute and ctx for this? I acknowledge it's a bit of a foot-gun.

sharksforarms commented 1 year ago

Thanks for working on this.

I like the idea. I wonder if it we could assume types aligned by default, then certain attributes may invalidate this assumption such as bits, reader, ..., WDYT?

If I understand correctly, this is to avoid the un-aligned allocation path right? (We can confirm via a test in test_alloc.rs)

wcampbell0x2a commented 1 year ago

I like the idea. I wonder if it we could assume types aligned by default, then certain attributes may invalidate this assumption such as bits, reader, ..., WDYT?

I think this could be hard to track, as alignment in the parent struct could effect the alignment of the child struct. I added a test case in which case this causes a panic b/c of lack of alignment: https://github.com/sharksforarms/deku/pull/295/files#diff-d0c3d1ab0ac5b9134d620e8e6f9574d3f0545b36474f310912b2a85ad6ded372R37

I actually think in most cases, you would set the alignment maybe in the top-level(something I haven't implemented).

#[deku(endian = "big", aligned)]
struct DekuTest {
    field_a: u8,
    field_b: u8,
    field_c: u16,
}