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

filter: Remove manual memory management #3619

Merged
merged 1 commit into from Jul 5, 2020

Conversation

ThomasHabets
Copy link
Contributor

Mostly manual work, followed by updating checksums.

Note that there were (and still remain) a lot of default copy
constructors and copy assignment operators in the codebase that are
not safe. And they're even exported to Python (some removed in this
commit).

While it's perfectly possible to allow safe copies once raw pointers
are removed, it seems like a performance trap waiting to happen. Hence
I removed the fir_filter copy cons/assignment.

Short script I used to update the header file hashes for pybind11:

a=polyphase_filterbank
S=$(md5sum $(find -name "${a}.h") | awk '{print $1}')
sed -i -r "s/(BINDTOOL_HEADER_FILE_HASH)[(].*[)]/\1($S)/" $(find -name "${a}_python.cc")

Mostly manual work, followed by updating checksums.

Note that there were (and still remain) a lot of default copy
constructors and copy assignment operators in the codebase that are
not safe. And they're even exported to Python (some removed in this
commit).

While it's perfectly possible to allow safe copies once raw pointers
are removed, it seems like a performance trap waiting to happen. Hence
I removed the fir_filter copy cons/assignment.

Short script I used to update the header file hashes for pybind11:

```
a=polyphase_filterbank
S=$(md5sum $(find -name "${a}.h") | awk '{print $1}')
sed -i -r "s/(BINDTOOL_HEADER_FILE_HASH)[(].*[)]/\1($S)/" $(find -name "${a}_python.cc")
```
@ThomasHabets
Copy link
Contributor Author

Oh right: Note the const_cast in fir_filter. This was always there. It's just that T* member variable becomes const T* when object is const, not const T* const, which std::vector correctly does.

There's no good way out of that cast, I think. It needs to be mutable, and allocated dynamically and not in the hot path. Alternatively: can VOLK provide a stack-allocated aligned buffer? a volk::array<> or something?

@marcusmueller
Copy link
Member

Oh right: Note the const_cast in fir_filter. This was always there. It's just that T* member variable becomes const T* when object is const, not const T* const, which std::vector correctly does.

yeah...

There's no good way out of that cast, I think. It needs to be mutable, and allocated dynamically and not in the hot path. Alternatively: can VOLK provide a stack-allocated aligned buffer? a volk::array<> or something?

No, currently VOLK doesn't (I don't know whether it should – although we're "abusing" it in high-level code, I'd say volk_malloc is really a low-level function; frankly, if we want to make full use of it, we might want to write an STL allocator using it.

@marcusmueller marcusmueller merged commit d20a773 into gnuradio:master Jul 5, 2020
//
// This prevents accidentally doing needless copies, not just of fir_filter,
// but every block that contains one.
fir_filter(const fir_filter&) = delete;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has broken MSVC + clang-cl build for me. From clang-cl while compiling e.g. filterbank.cc and others:

In file included from f:\gv\VC_2019\VC\Tools\MSVC\14.27.29110\include\string:11:
In file included from f:\gv\VC_2019\VC\Tools\MSVC\14.27.29110\include\xstring:14:
f:\gv\VC_2019\VC\Tools\MSVC\14.27.29110\include\xmemory(694,76): error: call to deleted constructor of
      'gr::filter::kernel::fir_filter<std::complex<float>, std::complex<float>, float>'
        ::new (const_cast<void*>(static_cast<const volatile void*>(_Ptr))) _Objty(_STD forward<_Types>(_Args)...);
                                                                           ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~
f:\gv\VC_2019\VC\Tools\MSVC\14.27.29110\include\xmemory(1508,35): note: in instantiation of function template
      specialization 'std::_Default_allocator_traits<std::allocator<gr::filter::kernel::fir_filter<std::complex<float>,
      std::complex<float>, float> > >::construct<gr::filter::kernel::fir_filter<std::complex<float>,
      std::complex<float>, float>, gr::filter::kernel::fir_filter<std::complex<float>, std::complex<float>, float> &>'
      requested here
        allocator_traits<_Alloc>::construct(_Al, _Unfancy(_Last), _STD forward<_Types>(_Vals)...);
                                  ^
...
F:/gv/dx-radio/gnuradio/gv-build/include\gnuradio/filter/fir_filter.h(41,5): note: 'fir_filter' has been explicitly
      marked deleted here
    fir_filter(const fir_filter&) = delete;
    ^

In my lack of C++ knowledge, I fail to understand what the problem is. It seems to work with GNU-C++ since this change has been there for almost 4 months.

But trying a:

    fir_filter (const fir_filter &);
    fir_filter (fir_filter &&) = default;
    fir_filter & operator = (const fir_filter &);
    fir_filter & operator = (fir_filter &&) = default;

causes 6 unresolved symbols during link.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing conversation in #3757

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants