Array of shared pointers - I'm flounding and could use some help!

Hi everyone,

I did my best to search Stack Overflow for an answer, but I’m still not having any luck. Previously I had an array of structures like so:

Sample samples[NUMBER_OF_SAMPLES];

But I’m starting to do some crafty things with that array which has required me to convert them to an array of pointers to Samples:

Sample *samples[NUMBER_OF_SAMPLES];

Just to give you a little bit of background, I’m making an expansion module to one of my modules where a pointer to a Sample structure is passed from the expansion module to the main module. Everything has been going relatively smoothly, but there’s some trickiness in how the pointers are managed.

I think my code is crashing because I’m trying to free up Sample pointer in one module, but it’s still being used in the other module.

That’s when I ran across this article: https://stackoverflow.com/questions/23883236/avoid-program-crash-when-deleting-pointer

And the most popular answer was essentially, “You should be using smart pointers.”

So I thought, sure, I can try that. I learned C++ 20+ years ago and I haven’t been exposed to smart pointers. I think that I should use shared pointers. So I tried creating an array of shared pointers using the following syntax:

std::shared_ptr<Sample[]> samples (new Sample[NUMBER_OF_SAMPLES]);

But I got an error message:

std::shared_ptr<Sample[]> samples (new Sample[NUMBER_OF_SAMPLES]);
error: expected ‘,’ or ‘…’ before ‘new’

It would get pretty involved if I were to explain all the details of my particular situation. But here’s exactly where it is crashing. I don’t expect this to make much sense without a lot of explaination. If more details would help, I’d be happy to dive into it.

          Sample *old_sample = this->samples[sample_slot];
          this->samples[sample_slot] = expander_message->sample_ptr;
          if(old_sample != NULL) delete old_sample; // crashing!

I think - may be if I can get an array of shared pointers working it will solve everything. :slight_smile:

I could use any advice you might have.

Cheers
Bret

Crash on delete means it’s either a wild pointer or it’s already deleted. And it’s win the standard that you can delete a null pointer so I you don’t need the test before delete.

I don’t think smart pointers are going to help you,since your problem is probably deleting too much, not leaking memory.

That first compiler error looks like you didn’t include all the headers you need.

Yeah, I totally agree. For a short amount of time, both the expansion module and the main module are using the pointer to the sample data. And shortly after that, it’s ready to be released. But neither module appears to be able to delete the memory referenced by the pointer without VCV Rack crashing. I’ll keep looking at it, but so far I’m stumped.

I don’t think you will get it working reliable this way. Even if you solve this problem I can imagine that many more of this kind will follow…
You are in a multithreaded environment and you can’t reliable predict the „order“ of the process calls of main-module and expanders. Even worse, the data could be modified somewhere else between your if and the delete.
It might get very ugly soon but I would modify the data only in the main-module, so the expanders need to pass a message what to be deleted through the expander mechanism.

I like that idea and I’ll try it tomorrow. I was thinking along the same lines. :slight_smile:

That syntax in your first post is not an array of smart pointers. Its a shared pointer to an array.

The expander module and the main module should not both have access to the objects at the same time. That’s why you should double buffer Create two memory objects, and swap them (every frame if necessary) using the flip request.

If the objects need to last longer than that, then track them in odd or even generations. They might end up living for one frame longer than necessary, but you can synchronise reads and writes that way.

1 Like

Oh and if you really want to do it right NEVER call new & delete in the audio thread. Memory allocation can take an indeterminate amount of time. You ESPECIALLY don’t want to have allocation and deletion happening in every Process call. New and delete are thread safe, so there’s a lock/unlock of the memory pool every time you do it, further increasing overhead.

If possible, make all allocation and deletion happen in the GUI thread.

And you’ll also have to put a lock around new/delete if different threads might be doing that. And putting locks in Process is also not a great idea unless they’re absolutely necessary.

I don’t 100% understand what you’re trying to do here, but it would make sense to try and move all the allocation/deletion to the GUI thread. You could have a manager object iwth an “eventually free” member function, that does a lock, adds the pointer to a std::set, and unlocks. That would take a deterministic and small amount of time. Insert into a std::set is idempotent, meaning that set members are unique and adding a value that’s already there doesn’t do anything.

2 Likes

Let me see if I can do a decent job explaining it. I’ve written a module called “Grain Engine MK2”, which does granular synthesis. You use it by loading up to 5 .wav files from disk. I’m building an expansion module that can record incoming audio from other modules and, after recording has completed, pass that audio into Grain Engine MK2 for processing. Grain Engine MK2 can’t record audio, so this is a cool new feature.

My initial implementation was a little sloppy. The expansion module would record the audio, write it to a .wav file, then pass the filename of the saved data to Grain Engine MK2, which would load it into one of the 5 sample slots. One nice thing about this approach is that restarting VCV Rack wouldn’t cause the recorded audio to be lost since Grain Engine MK2 treated it like any other .wav file.

I’ll get back to that. Here’s a bit more background on the code…

Grain Engine MK2 contains an array of 5 Samples that load .wav file data into vectors for playback. Here’s some code to illustrate:

// Sample.hpp : Part of the definition that shows the audio buffers

struct Sample
{
  std::vector<float> leftPlayBuffer;
  std::vector<float> rightPlayBuffer;
  ... etc
// GrainEngineMK2 : Showing the array of 5 Sample structures

Sample *samples[NUMBER_OF_SAMPLES];

// In the constructor:
for(unsigned int i=0; i<NUMBER_OF_SAMPLES; i++)
{
  samples[i] = new Sample();
}

Everything was working great, but I knew that saving and loading a .wav file is really time consuming, and I felt guilty about that solution! I figured it might cause trouble. Then it dawned on me… the expansion module also used a Sample to store the information it was recording:

// Expansion Module : Uses a single Sample for storing audio information
Sample *sample = new Sample();

I won’t go deeply into how the two modules communicate unless asked. On the surface level, here’s the structure for messages sent from the expander to the main module.

struct GrainEngineExpanderMessage
{
  unsigned int sample_slot = 0;
  bool message_received = false;
  Sample *sample = NULL;
};

// In the expander module:
// GrainEngineExpanderMessage *message_bus = (GrainEngineExpanderMessage *) rightExpander.module->leftExpander.producerMessage;


So my thought was: Can I pass in a pointer to the expansion module’s Sample, which contains the audio data, into the main module? Or, put another way, instead of forcing Grain Engine MK2 to load the saved audio data, how about just sending over a pointer to it?

GrainEngineMK2 has 5 sample slots. So I would be essentially saying, Hey Grain Engine MK2, you know sample slot #4? I have a new Sample for you to use for that slot. So ditch the old one and here’s a pointer to the new one for you.

Juggling the pointers got a bit tricky. Here’s how I envisioned that it would work.

  1. You record some audio in the expansion module, which pushes the audio information into the Sample’s vectors (leftPlayBuffer & rightPlayBuffer)

  2. The expansion module sends a pointer to the Sample to the main module along with which sample slot to overwrite.

  3. The expansion module then saves the recorded audio to disk.

  4. After saving the audio, the expansion module “abandons” that pointer and creates a new empty sample for it to use.

// In the expansion module
message_bus->sample = sample;  // pass the Sample to the main module
sample->save_recorded_audio(sample->path + sample->filename);  // save audio to a file
sample = new Sample();  // Get a fresh new sample to record more audio into

On the Grain Engine MK2 side of things, it sees an incoming message and does the following:

  1. Receives the sample slot (0-4) of where to store the incoming sample information

  2. Deletes the old Sample structure at that slot

  3. Assigns the sample pointer at that slot to the incoming sample pointer

Maybe it’s easier to understand the code:

// In Grain Engine MK2
unsigned int sample_slot = expander_message->sample_slot;
Sample *old_sample = this->samples[sample_slot];        // Me being paranoid
this->samples[sample_slot] = expander_message->sample;  // load in new sample coming from expansion module
delete old_sample;  // Delete old sample being overwritten by new sample.  HERE IS WHERE IT IS CRASHING

If I remove delete old_sample; it stops crashing, but I suspect that would cause a memory leak.

I haven’t figured out why it crashes when I attempt to delete the old sample. I’m probably overlooking something. Any help would be much appreciated.

Cheers,
Bret

I wanted to add two things:

  1. @chaircrusher I am listening to you when you recommend moving memory allocation stuff into the GUI thread. I need to learn more about that for sure, but I’ll dig into that a little later.
  2. I was able to confirm that if I don’t delete the pointer to the old Sample that the module does not free up the memory.

I’m starting to get to the bottom of this. If the audio recorded into my vectors is short (1/2 second or less), everything works fine. But if it’s longer, deleting the struct containing the vectors crashes. I’ll post my solution if I figure one out.

The length of the recording may be a red herring.

It may be that when the audio is short, stuff is still going wrong, or still has the potential to go wrong but is simply not triggering a crash. Maybe memory is being overwritten which is not particularly important at the time. But a different length recording is leaving the ‘at risk’ area somewhere where it matters.

The problem with doing the allocation / deallocation in the GUI thread, is that in rack v2 there’s no guarantee that you will have any code running in the GUI thread at all.

For time critical applications, it is sometimes worth considering trading time for space. Pre-allocating a chunk of memory of sufficient size, and then re-using it over and over again.

2 Likes

Thank you David. Pre-allocating memory is brilliant! I’ll have to try that. Hmm… I’ll have to put a cap on the sample length - which isn’t a huge problem since my modules work best with shorter samples anyhow. Maybe 6 seconds max. I think that’s a bit over 6 megabytes for 5 samples.

My other idea was to never delete the Sample pointers but instead swap them around so there’s always 6 of them (5 for the Grain Engine and 1 for the expander.)

I also appreciate the red herring statement. That’s very likely the case.

Cheers, Bret

agreed. some of these suggestions really have me scratching my head, not just for v2, but for v1 as well. the GUI thread is already fairly constrained on some systems, and relying on it for things like memory allocation is a bad idea.

if you must be allocating and freeing memory, create your own thread.

Won’t work because shared_ptr calls delete in C++11, not delete[]. Doing this is incorrect code and if it compiles at all, it may segfault. In general, don’t use shared_ptr in DSP code. There’s no valid use of it and suggests that your design is bad. Anyone is welcome to prove me wrong, but I believe any problem in DSP that can be solved with shared_ptr can be solved instead with raw pointers with a good pointer ownership design, producing better code that is easier to prove its correctness.

Nonsense. Think for yourself, don’t listen to random people on the internet make blanket statements.

You’re massively overcomplicating this. If you want to send a large buffer using the module expander API, you have two options:

  • Design a message-passing protocol to send the entire buffer when it changes, so that both modules own a copy.
  • Declare that the mother module “owns” the buffer. Read it directly by the expander module every frame with
if (leftExpander && leftExpander->model == modelMother) {
    float sample = reinterpret_cast<Mother*>(leftExpander)->buffer[bufferIndex];
}

In general, don’t use shared_ptr in DSP code. There’s no valid use of it and suggests that your design is bad.

I’m honestly relieved to hear this. :slight_smile:

Declare that the mother module “owns” the buffer. Read it directly by the expander module every frame with …

This solution sounds similar to @carbon14’s response and I think it’s a great idea. If it works, I’ll forever wonder why my previous solution was crashing. But I do this as a hobby, so I’ll take what I can get!

Nonsense. Think for yourself, don’t listen to random people on the internet make blanket statements.

Yikes, what a riddle! A conundrum to be certain. (chews on nails nervously) Ha ha ha.

2 Likes

if you’re developing on anything but windows, you can use address sanitizing to help track down memory issues in the future.

1 Like

The point about the GUI thread not existing for headless operation is very true, forget I suggested it. But your allocation/reallocation need to be thread safe.

The solution of writing to disk and passing the name isn’t that bad, actually. It will be less of a performance hit than you might think.

The crash may be as simple as using delete when you should use delete [].

1 Like

That’s good news. I may fall back on that for the short term. It’ll save me a lot of complexity and risk. But I may give my current endeavor one more shot. :slight_smile:

@chaircrusher I just restored the old behavior of writing to disk and passing the filename. It’s working beautiful again and is super stable, so I think I’ll stick with that solution.

THANK YOU EVERYONE for your insights. I really appreciate it. :bowing_man:

1 Like