1

I'd like to ask about your opinion for a code I've figured out.

Context

I'm implementing a queue as a ring buffer. Each queue member is a struct below:

struct member {
  int size;
  char data[]
};

What's important in my case: for each member, the same amount of data should be allocated.

Why don't just provide array size if it's the same for every element?

I want to use multiple queues with different parameters, and just provide number of elements and max size of element buffer as arguments.

Why don't just have pointer in each entry and malloc individually?

I'd like to improve performance and store everything in one memory block, rather than multiple blocks at multiple addresses

My solution

I've got and idea how to make it work, based on 1 assumption: Member data size must be a multiple of 4 - to avoid any padding that could mess up address calculation

NOTE Macros are set just for example. I don't know the real sizes at the compile time

#define DATA_SIZE 64
#define N_OF_STRUCTS 3

struct entry {
  int size;
  char data[]; // 64
};

struct entry *get_at_index(struct entry *buff, int index) {
  return buff +
         index * (offsetof(struct entry, data) + DATA_SIZE * sizeof(char));
}

int main() {
  struct entry *s0 =
      malloc((sizeof(int) + sizeof(char) * DATA_SIZE) * N_OF_STRUCTS);

  struct entry *s1 = get_at_index(s0, 1);
  struct entry *s2 = get_at_index(s0, 2);

  (s0 + sizeof(int) + DATA_SIZE)->data[0] = 'a';
  (s0 + 2 * (sizeof(int) + DATA_SIZE))->data[0] = 'b';
  assert(s1->data[0] == 'a');
  assert(s2->data[0] == 'b');
  
  printf("%s\n", s1->data);
  printf("%s\n", s2->data);

  return 0;
}

What I'd like to know

I'd like to ask what's your opinion on that.

  • Is there something I didn't consider?
  • Would it work on other architectures?
  • Is this anti-pattern and if so, why?
g3t0r
  • 13
  • 5
  • There's a couple of problems: The `%s` format expects an argument that is a null-terminated string. You never make any of the `data` members a null-terminated string. Your code also makes assumptions about the memory layout that are implementation specific, like that there's no padding in the structure. There are probably more problems in the show code. Why are you doing it the way you're doing it, with all that pointer arithmetic and `offsetof` etc.? The size of each structure is `sizeof(struct entry) + DATA_SIZE`. It's as simple as that. – Some programmer dude Aug 27 '23 at 16:30
  • As for the `get_at_index` function, it could just just treat the pointer as a pointer to bytes (which is really what it is) and then use `index * (sizeof(struct entry) + DATA_SIZE)` as the byte-index to the structure position, and return a pointer to that. – Some programmer dude Aug 27 '23 at 16:56
  • @Someprogrammerdude The printf was just for a test. My main question is regarding allocation and access. I thought that by using int (4 bytes) and then data size as multiple of 4 I can avoid padding, which should make it work with all implementations. Is it more complicated? – g3t0r Aug 27 '23 at 17:01
  • Padding or not is an implementation detail. Unless you know exactly what your compiler will do, and will never try to use any other compiler (or even any other version of the same compiler), then you can use "tricks" like that. But doing such "tricks" isn't a good habit, and it's not something that anyone around you would want to handle. If you checked in code like that in my team, the code-review would get downvoted quite quickly. – Some programmer dude Aug 27 '23 at 17:34
  • This would be better asked on the CodeReview exchange. – Daniel Walker Aug 27 '23 at 17:42
  • @DanielWalker I'm not sure about that. I don't really care about the code above, I rather care about a concept of storing data in memory like this. But thanks, didn't knew CodeReview Exchange exits, for sure I'll use it in the future :) – g3t0r Aug 29 '23 at 19:14

2 Answers2

1

This won't do as you expect:

(s0 + sizeof(int) + DATA_SIZE)->data[0] = 'a'; 
(s0 + 2 * (sizeof(int) + DATA_SIZE))->data[0] = 'b';

Because pointer arithmetic is done in terms of multiples of the size of the pointed-to object. For example s0 + sizeof(int) + DATA_SIZE doesn't advance the pointer by sizeof(int) + DATA_SIZE bytes but by sizeof(struct entry) * (sizeof(int) + DATA_SIZE) bytes. So you won't be accessing the element you think you are.

The same problem exists here:

return buff +
     index * (offsetof(struct entry, data) + DATA_SIZE * sizeof(char));

And has an additional issue, namely that it doesn't account for any padding the might exists between the size and data members.

So there are two main things you need to do here:

  • Use a char * when offsetting the array so pointer arithmetic moves 1 byte at a time.
  • Since sizeof(struct entry) takes into account any padding that might exits between the members but not the flexible array member, your offset just needs to be sizeof(struct entry) * DATA_SIZE;
struct entry *get_at_index(struct entry *buff, int index) {
  return (struct entry *)((char *)buff +
         index * ((sizeof(struct entry) + DATA_SIZE));
}

For alignment issues, if you want to allow DATA_SIZE to not be a multiple of 4, you can add trailing padding:

int PAD = (DATA_SIZE % _Alignof(struct entry)) ? 
              (_Alignof(struct entry) - (DATA_SIZE % _Alignof(struct entry))) : 
              0;

struct entry *get_at_index(struct entry *buff, int index) {
    return (struct entry *)((char *)buff +
           index * ((sizeof(struct entry) + DATA_SIZE + PAD));
}

int main() {
  struct entry *s0 =
      malloc(sizeof(struct entry) + DATA_SIZE + PAD) * N_OF_STRUCTS);
  ...
dbush
  • 205,898
  • 23
  • 218
  • 273
0
  struct entry *s0 =
      malloc((sizeof(int) + sizeof(char) * DATA_SIZE) * N_OF_STRUCTS);

Is bad as you do not know how your implementation will store your struct int the memory.

  struct entry *s0 =
      malloc((sizeof(*s0) + DATA_SIZE) * N_OF_STRUCT);

Also pointer arithmetic does not happen on the byte (or char) level. You need to learn more about pointers and pointer arithmetic

Generally, your idea does not make too much sense as you use macro definitions for sizes and it is known at compile time. It would make sense if your datatypes were created at run time (so nothing is known when the code is compiled). It looks like an XY problem to me.

I want to use multiple queues with different parameters, and just provide number of elements and max size of element buffer as arguments.

You cant as the sizes are have to be known during compilation.

I'd like to improve performance and store everything in one memory block, rather than multiple blocks at multiple addresses

How can you know if it will have any effect? Did you profile the application? Remember: enter image description here

To have an ability to create those data runtime not having defined data sizes and queue length you need to store a bit more data.

Example allocating one large chunk of memory:

typedef struct 
{
    size_t queuesize;
    size_t elementsize;
    size_t *sizes;
    unsigned char *data;
    unsigned char storage[];
}queue_t;


typedef struct 
{
    size_t size;
    unsigned char *data;
}entry_t;


queue_t *createQueue(size_t size, size_t datasize)
{
    queue_t *q = malloc(sizeof(*q) + size * (datasize + sizeof(q -> sizes[0])));
    if(q)
    {
        q -> sizes = q -> storage; 
        q -> data = q -> storage + sizeof(q -> sizes[0]) * size;
        memset(q -> sizes, 0, sizeof(q -> sizes[0]) * size);
        q -> queuesize = size;
        q -> elementsize = datasize;
    }
    return q;
}

entry_t get_at_index(queue_t *q, size_t element) 
{
    entry_t t;
    t.size = q -> sizes[element];
    t.data = q -> data + q -> elementsize * element;
    return t;
}

void put_at_index(queue_t *q, size_t element, size_t datasize, void *data) 
{
    q -> sizes[element] = datasize;
    memcpty(q -> data + q -> elementsize * element, data, datasize);
}

0___________
  • 60,014
  • 4
  • 34
  • 74
  • Macros are just for an example. Why I don't know how it will store it in memory? If I prevent struct padding, I thought it will be as it is – g3t0r Aug 27 '23 at 16:56
  • @g3t0r if you prevent padding the toy very likely will kill performance. 2. It is wrong example as this code will only work with macros – 0___________ Aug 27 '23 at 16:59
  • By preventing I meant that I thought they will not need it, not that I'm forcing lack of padding. Am I wrong about that? – g3t0r Aug 27 '23 at 17:18
  • @g3t0r you are wrong about many things. 1. Your idea can't be used to create your queues runtine (at least queues with different data sizes) 2. Your pointer arithmetic is wrong 3. Your struct understanding is wrong. I gave you an example how it can be done. – 0___________ Aug 27 '23 at 17:22
  • _How can you know if it will have any effect? Did you profile the application?_ Tbh I've just heard that it improves performance because it should be in the same cache (I think it was L3). I'm pretty new to the topic. I guess it's more 'it depends' than 'for sure it will improve performance'? – g3t0r Aug 27 '23 at 17:22
  • @g3t0r do not think about premature optimizations. – 0___________ Aug 27 '23 at 17:33