Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Remove use variable length array #2279

Merged

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_;
    // ...
}

@maxsharabayko
Copy link
Collaborator

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

srtcore/epoll.cpp Outdated Show resolved Hide resolved
srtcore/epoll.cpp Outdated Show resolved Hide resolved
@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