Skip to content

Conversation

quink-black
Copy link
Contributor

No description provided.

@maxsharabayko
Copy link
Collaborator

I would propose using the FixedArray then (from utilities.h).
It can't (and is not expected to) be resized, unlike std::vector. Even though in this case of a local variable it is not that important.

@maxsharabayko maxsharabayko added this to the v1.4.5 milestone Apr 5, 2022
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Apr 5, 2022
@quink-black
Copy link
Contributor Author

I would propose using the FixedArray then (from utilities.h). It can't (and is not expected to) be resized, unlike std::vector. Even though in this case of a local variable it is not that important.

Could you give some hints about the benefits of FixedArray other than forbidden of resize? There is no reallocation here, so I guess the overhead of std::vector can be negligible.

@maxsharabayko
Copy link
Collaborator

Could you give some hints about the benefits of FixedArray other than forbidden of resize? There is no reallocation here, so I guess the overhead of std::vector can be negligible.

It is a perfectly fair question, but I have no unambiguously correct answer to it. I will give my thoughts, while I do not insist on using the FixedArray. Especially given we talk about a local variable with a short lifetime.

In this very place, we want to have an array of epoll_event. The ideal would be to use std::array, except we don't know the size at compile time. Therefore we need an array allocated in run-time, preferably destructing itself properly, so we don't need to call delete [].
Based on this description a conventional guideline is to use std::vector.

However, std::vector is much more powerful than just a dynamically allocated array. Thus we get two disadvantages:

  1. If a non-const std::vector is used, we don't have a guarantee that somewhere it will not be resized. The use of vector itself does not provide this intent to the reader of the code. In this case of a local scope - not a big deal.
  2. Slightly extra footprint. We need a pointer and array size, we get a bit more. E.g. see the below implementation of the std::vector having two-pointers and an additional capacity pointer. (The FixedArray currently also has an extra pointer pointing to an error string I brought in PR [core] Fixed std::runtime_error usage (use C++03 version instead of C++11) #2184. A quick and dirty fix to be re-worked.)

Thus in this "proposal" my main motivation is the first item from the list above: to show intent it is just an array we don't plan to resize.

template <class _Tp, class _Allocator>
class __vector_base
    : protected __vector_base_common<true>
{
    // ...
    pointer                                         __begin_;
    pointer                                         __end_;
    __compressed_pair<pointer, allocator_type> __end_cap_;
    // ...
}

@quink-black quink-black force-pushed the remove-variable-length-array branch from d545b83 to 5e914de Compare April 6, 2022 15:52
@quink-black quink-black force-pushed the remove-variable-length-array branch from 5e914de to 5d70291 Compare April 7, 2022 02:38
@maxsharabayko
Copy link
Collaborator

PR #2298 adds srt::FixedArray::data() that (once merged) can be used here instead of &ev[0].

@maxsharabayko maxsharabayko modified the milestones: v1.5.0, Next Release May 24, 2022
@maxsharabayko maxsharabayko modified the milestones: v1.5.1, Next release Sep 12, 2022
@maxsharabayko maxsharabayko merged commit 38b4211 into Haivision:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants