roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.06k stars 211 forks source link

Add lengths to StringList #625

Closed ForeverASilver closed 10 months ago

ForeverASilver commented 11 months ago

pull request for #617

string lengths are stored as size_t and written to the buffer just before the string.

changed tests to create new expected allocation counts to account for different size_t sizes.

any suggested changes are welcome and appreciated

gavv commented 11 months ago

Thanks for PR!

A few suggestions.

1. I think the could be a bit cleaner if we introduce structs for header and footer:

class StringList {
...
private:
    struct Header {
        size_t len;
        char str[];
    };

    struct Footer {
        size_t len;
    };

Then we do something like this:

Header* header = (Header*)&data_[cur_sz];
Footer* footer = (Footer*)(header->str + header->len);

and then we can use header->len, footer->len, and header->str instead of char pointers.

To get pointer to Header from pointer to string x, we can use ROC_CONTAINER_OF(x, Header, str).

2. Instead of (8 - ((add_sz + len_sz) % 8)) we can use AlignOps::align_as()

3. Do we really need to store padding size? I think we can store unaligned length and calculate aligned length using AlignOps::align_as().

4. To keep tests simpler and to reduce memory usage a bit, I think it's OK to use uint32_t instead of size_t to store length.

ForeverASilver commented 11 months ago
  1. I agree adding structs would probably make this cleaner, currently working on it. for points 2, 3, and 4, i was not aware of AlignOps, hence the alignment to keep UBSan happy about accessing size_t on 8 byte boundaries, and the different expected allocations on 4 byte and 8 byte size_t in tests, will change this though to utilize AlignOps as you mentioned
gavv commented 11 months ago

@ForeverASilver please ping me when it's ready for review :)

ForeverASilver commented 11 months ago

@gavv ready for review!

implemented the Header and Footer structs as per your comments, though due to errors within ROC_CONTAINER_OF, had to do some a couple of const_cast.

ForeverASilver commented 10 months ago

@gavv ready for review!

gavv commented 10 months ago

Thanks!

Small follow-up commit: 46491cdb6e49a1b0755b78d73199d21234912835 (cosmetic fixes)