srtp_stream_list_ctx_t_: standarize naming

[jmillan]

use 'capacity' and 'size' in order to represent the allocated storage and the number of elements respectively.

As per @Lastique comments here

[Lastique]

Unrelated to this PR, but you may want to consider changing the grow factor of the array to 1.5 (i.e. to size_t new_capacity = list->capacity + (list->capacity + 1u) / 2u;). See here for a discussion why.

Also, while at it, you may want to check for capacity overflow, i.e. if (new_capacity <= list->capacity) return srtp_err_status_alloc_fail;.

[jmillan]

@Lastique, nice comments. I'll do some modifications in this PR.

[Lastique]

Also, while at it, you may want to check for capacity overflow, i.e. if (new_capacity <= list->capacity) return srtp_err_status_alloc_fail;.

Actually, this condition is not quite correct. It should be something like this: if ((new_capacity * sizeof(list_entry)) <= (list->capacity * sizeof(list_entry))) return srtp_err_status_alloc_fail;.

[jmillan]

The PR is now ready for review.

[jmillan]

I've rebased to a single commit in order to have a better git history.

[paulej]

Unrelated to this PR, but you may want to consider changing the grow factor of the array to 1.5 (i.e. to size_t new_capacity = list->capacity + (list->capacity + 1u) / 2u;). See here for a discussion why.

I read the material at the referenced link, but I fail to see the problem. If a vector has 100 elements and it grows by even 1, it will have to allocate a new buffer. If calling realloc(), there is a chance the buffer will remain in the same position, but just have additional memory available to the right of the existing buffer. In the worst case, it calls malloc() and moves all of the current data. What I fail to see is how this changes, regardless of C + 1, C * 1.5 ,or C * 2.

What am I missing?

[jmillan]

What am I missing?

This means that any new chunk requested will be larger than all previously used chunks combined, so the vector must crawl forward in memory; it can't move back to its deallocated chunks. But any number smaller than 2 guarantees that you'll at some point be able to reuse the previous chunks

A factor of 2 will never let you to reuse previously used chunks. That will also contribute to memory fragmentation.

[paulej]

it can't move back to its deallocated

Sure, but those chunks will be returned, and the OS will (ideally coalesce those into a larger contiguous chunk and) give those chunks to other application logic that requests memory. If the OS does not coalesce memory, then those smaller, previously allocated chunks will remain fragmented and could not be given to a vector that is growing (even at 1.5*C) since all chunks will be smaller than required size. If the OS does coalesce memory, sure: the vector could potentially be placed where it was before. In any complex system, though, it's likely that memory is going to be handed to something else, anyway. If there is no coalescing happening, the 1.5 * C allocations will actually produce more fragmentation.

[Lastique]

Sure, but those chunks will be returned, and the OS will (ideally coalesce those into a larger contiguous chunk and) give those chunks to other application logic that requests memory.

This will at best only happen when the allocator has a whole unused page to free to the OS, which may not happen soon, depending on the vector sizes and the number of allocations. And that is assuming the allocator actually does release the page and not e.g. cache it for potential reuse in the future. Either way, reusing the memory within the application, without round tripping to the OS, is more efficient. The point of the 1.5 grow factor is that we allow for such reuse to happen, whereas with the factor of 2 we would not. It doesn't mean such reuse will happen, though.

[jmillan]

@paulej I definitely cannot defend the 1.5 factor growth.

Since that change is unrelated to the initial PR intention we can go back to the initial factor 2 and apply the name changes and the capacity overflow check.

[paulej]

I have no strong objection either way. I was just confused by the claim. Now that I understand it better, I do think that claim, while not wrong on a pure sense, is wrong in reality since it doesn't take into account the fact that the system is doing lots of other things. It's more likely than not that previously freed memory is doing to be consumed by something else. However, either works and I cannot prescribe what the best growth factor is. I suspect the best answer here is what works best for a typical, growing multi-party conference. I don't think there is a good answer, unless we know a priori what to expect.

[jmillan]

I have removed the 1.5 factor and applied back the previous 2.

This PR does two things:

• Use standard names in two struct members: (size, capacity). This is a readability & maintainability enhancement.
• Verify capacity overflow prior to requesting memory. This is a good practice and ultimately a bug fix.

As I get from the comments, changing the increase factor requires its own space (PR) and corresponding rationale, data, etc.