Improve exception safety with smart pointers

[elfring]

Would you like to wrap any pointer data members with the class template “std::unique_ptr”?

Update candidates:

• FIRFilter
• GRULayer

[jatinchowdhury18]

@elfring, thanks for pointing these out!

IIRC, I actually was using smart pointers in both of these parts of the code a one point or another. I ended reverting to raw pointers after performance on some machines turned out to be faster with the raw pointers (I don't remember which machines... probably worthwhile to measure again). I've asked some more advanced C++ programmers about this, and the consensus is that using smart pointers "should be" just as fast as raw pointers. I've wondered if there's some compiler flags or something similar that I need to change to make sure the compiler optimises smart pointers to the fullest extent possible.

In the meantime, I'm not terribly worried about memory-safety in these parts of the code, since their construction/destruction code will only ever be called in a very controlled way. That said, I'd be happy to hear if you have any suggestions for improving the memory-safety/smart-pointer performance situation.

If you need to measure the performance yourself, I've been writing up small blurb on how to measure the performance using the ChowTape headless tool, but I haven't finished yet. Basically, you can build headless by running:

cmake -Bbuild -DBUILD_HEADLESS=True
cmake --build build --target ChowTapeModel_Headless

and then run the benchmarking tool with ./build/ChowTapeModel --bench. Note that you may need to adjust some parameter values, since I think the LossFilter which uses FIRFIlter is off by default. For more information, see here.

[elfring]

🤔 How do you think about to take the C++ guideline “R.11: Avoid calling new and delete explicitly” into account?

[jatinchowdhury18]

Yes, I'm aware that using new and delete in general is considered bad practice (and for good reason), hence why I use std::make_unique pretty much everywhere else in the code. If I remember correctly, in Stroustrup's "A Tour of C++", he mentions that while smart pointers improve memory-safety, they do introduce a tiny bit of computational overhead. In these parts of the code, the raw pointers point to an array of data that is beign accessed multiple times per sample, which in audio code means hundreds of thousands of times per second, so that overhead can add up to a noticeable amount.

For the moment, I'd say there are two things that would make me switch back to using smart pointers in these parts of the code:

1. It can be proven that the smart pointer implementation is at least as fast as the raw pointer implementation. This needs to be proven on a variety of compilers and machine architectures.

2. Some crashes, memory leaks, or otherwise undersirable behaviour in the plugin itself can be linked back to these calls to new and delete. Frankly, I think this is unlikely since the calls are being used in such a controlled manner. Consider (from the article you had linked):

In a large program, a naked delete (that is a delete in application code, rather than part of code devoted to resource management) is a likely bug

Relatively speaking, this is not a very large program, and the new and delete calls are only ever made in the "part of code devoted to resource management".

---

All that said, I would love to be proven wrong! Have you experienced any issues with the plugin related to memery allocation? Or, in your experience, are there tricks to using smart pointers to handle arrays without introducing this overhead?

[elfring]

Have you experienced any issues with the plugin related to memery allocation?

I dare to point implementation details out for further development considerations.
You can choose how much you would like to care for exceptional situations according to the potentially affected software.

Or, in your experience, are there tricks to using smart pointers to handle arrays without introducing this overhead?

• Not directly according to experiences which I can share so far.
• Would you find it acceptable to use the data type “std::vector” at any places?

[jatinchowdhury18]

I dare to point implementation details out for further development considerations.
You can choose how much you would like to care for exceptional situations according to the potentially affected software.

Yes, and thank you for pointing them out! Sorry if I came off as rude or dismissive, that was certainly not my intention. I was simply trying to explain why these decisions were made originally (I think sometime in July?).

Would you find it acceptable to use the data type “std::vector” at any places?

I have tried this before for FIR filters (in a different codebase), and I remember abandoning that approach for similar performance considerations. The other approach I've tried has been using std::array and passing the length of the FIR filter as a template parameter. Unfortunately, that approach makes the code less flexible, since the length of the FIR filter is determined at run-time as a function of the sample rate.

I think what might be best for me to do is to write 3 implementations, one with raw pointers, one with smart pointers, and one with std::vector, and compare performance results on a variety of machines and compilers. I'll probably use this repo for the tests since it already has most of the infrastructure. I'll probably get around to this later in the week.

Thanks,
Jatin

[elfring]

Sorry if I came off as rude or dismissive, …

I did not interpret your feedback in such a direction.

I was simply trying to explain why these decisions were made originally …

The explanation approach was helpful to some degree.

[jatinchowdhury18]

So it took me a lot longer that I expected, but I was able to convince myself that I can do FIR filtering with std::vector without taking a performance hit on any of the main platforms that I expect the plugin to be running on. This change has been committed to the develop branch: 2383e08.

The raw pointers in the RTNeural code are now gone as well, since I'm using an external library there instead. The library has its own issues with raw pointers, but that'll be a task for another day...