Make more ringbufs zero-initialized, saving flash

[cbiffle]

To simplify its implementation, ringbuf currently requires every ringbuf to be declared with an initialization element. It creates a static array containing that element.

If that element is represented as zero, that goes in BSS and is zero-initialized with the rest of BSS.

If that element is represented as anything other than zero, the compiler creates a full-sized "initialization image" copy of the ringbuf in flash that it can then memcpy into RAM on startup.

Using the default Rust enum representation, the easiest way to get a ringbuf initialized to non-zero bytes is to put its initialization sentinel value (e.g. None) as the last variant in the enum. Unfortunately, this is super common.

This commit moves all the ones I could find, which should reduce flash usage in a bunch of cases.

[hawkw]

Looks good to me!

For what it's worth, [if the enum has a repr attribute, you can also explicitly set the discriminant for that variant to 0][1]:

#[derive(Copy, Clone, PartialEq)]
#[repr(u8)]
enum Trace {
    None = 0,
    Whatever(u64)
}

or similar. IMO this might be worth considering since it feels slightly less like Hoping The Compiler Does The Right Thing Because We Put Them In Order, but 🤷‍♀️.

It might be worth adding a note about this in the ringbuf docs, so that folks adding new ringbufs in the future don't mess it up?

[cbiffle]

Hmmmm. Doesn't the repr attribute affect niche optimization? I'm concerned it might inflate the ringbuf statics.

[hawkw]

Hmmmm. Doesn't the repr attribute affect niche optimization? I'm concerned it might inflate the ringbuf statics.

Mmm, it might --- the current thing is fine, we can look into whether the explicit discriminant is worth doing separately.

[aapoalas]

Hmmmm. Doesn't the repr attribute affect niche optimization? I'm concerned it might inflate the ringbuf statics.

It does not, assuming you don't have enums like this:

#[repr(u8)]
enum A {
  None,
  Some(B),
}

#[repr(u8)]
enum B {
  X,
  Y,
}

I think here the repr enum layout might guarantee that X is 0 and Y is 1, which then means that the compiler cannot "flatten" A to have values 0/1/2.

I was also going to suggest considering repr enums: it would be a little easier to look for "mistakes" like this by searching for " None," in the codebase.

[cbiffle]

I think if I'm going to put much more thought into it, I'd rather eliminate the need for a sentinel value and default ringbufs to uninit. Having a None in an enum that isn't Option kind of bothers me, plus you can currently write ringbuf_entry!(Trace::None); and that produces silly results.

[hawkw]

I think if I'm going to put much more thought into it, I'd rather eliminate the need for a sentinel value and default ringbufs to uninit. Having a None in an enum that isn't Option kind of bothers me, plus you can currently write ringbuf_entry!(Trace::None); and that produces silly results.

Yup, agreed , making them uninitialized would be a nicer solution. Let's do that at some point.

[aapoalas]

I think if I'm going to put much more thought into it, I'd rather eliminate the need for a sentinel value and default ringbufs to uninit. Having a None in an enum that isn't Option kind of bothers me, plus you can currently write ringbuf_entry!(Trace::None); and that produces silly results.

That definitely sounds like the correct way to solve the whole problem, and not even too hard to do. Out of curiosity though, would you have a temporary counter for unset values to avoid reading the uninitialised data? Or would you somehow try to force ringbuf enums to not use the 0 value, so the bss 0 initial values could be read as uninitiated dynamically?

[cbiffle]

would you have a temporary counter for unset values

@aapoalas there are a couple options -- keeping a size that saturates at the size of the table would do it, for example. That'd cost one more word of memory, but it's probably worth it.

There are alternatives that could let you pack this information into the existing "position" counter used to track the position in the circular buffer, but they'd save at most one word, and are potentially fragile. (Example: use position==0 to indicate empty, store all real positions as x+1)

[aapoalas]

would you have a temporary counter for unset values

@aapoalas there are a couple options -- keeping a size that saturates at the size of the table would do it, for example. That'd cost one more word of memory, but it's probably worth it.

There are alternatives that could let you pack this information into the existing "position" counter used to track the position in the circular buffer, but they'd save at most one word, and are potentially fragile. (Example: use position==0 to indicate empty, store all real positions as x+1)

Thank you for the answer, much appreciated <3

I was actually just going to edit my comment as I was looking at the internals of ringbuf and realised I hadn't understood that there's metadata on each entry as well. That kind of changes the equation a little: one alternative would then be to use an Option<NonZeroUN> as the generation value. That'd avoid the word cost of the saturating counter in exchange for... I guess mildly more complicated code?