Is IComponent::getState()/setState() thread-safe?

SDK for VST 3 audio plug-in and host development.
User avatar
pongasoft
Posts: 84
Joined: Sun Mar 11, 2018 5:57 pm
Location: Las Vegas, USA
Contact:

Re: Is IComponent::getState()/setState() thread-safe?

Post by pongasoft » Sun Jun 17, 2018 12:49 pm

Arne Scheffler wrote:Please watch the good and funny video from Pete Goodliffe about the "The Golden Rules of Audio Programming" @ ADC. Then you hopefully know why you never ever use a lock in a realtime audio thread.

https://www.youtube.com/watch?v=SJXGSJ6Zoro

Cheers,
Arne
I did watch the video and ironically the section after "never ever use locks" is "never ever allocate memory" in realtime audio thread... (which is something I am particularly familiar with since in the Propellerhead Rack Extension SDK, allocating memory in the realtime audio thread is actually forbidden and will crash the plugin...)

So I guess I am back to my original point/issue. The solution you are suggesting obviously works (I did end up implementing it) but it does allocate and free memory in the realtime audio thread.

In the video, the issue with locks is that they can cause deadlocks, start small and then eventually have a really huge amount of code wrapped in the lock. Which are fine concerns but seems to me that they can be alleviated by good practice.. if a lock is only used for copying the state object (easy to encapsulate this by even simply using a std::atomic<State>...) then I don't see the concern. The memory allocation is a much more serious issue as you can't control what is going on under the cover... if the implementation uses a lock or need to defragment etc...

Definitely not an easy topic :(

Yan

Arne Scheffler
Posts: 291
Joined: Mon Jun 20, 2016 7:53 am

Re: Is IComponent::getState()/setState() thread-safe?

Post by Arne Scheffler » Sun Jun 17, 2018 2:48 pm

If you look again on my example, the memory allocations are all done outside the audio thread. I've not checked your code, but if you do allocations on the audio thread there then its your implementation you need to change.

Cheers,
Arne

User avatar
pongasoft
Posts: 84
Joined: Sun Mar 11, 2018 5:57 pm
Location: Las Vegas, USA
Contact:

Re: Is IComponent::getState()/setState() thread-safe?

Post by pongasoft » Sun Jun 17, 2018 3:35 pm

Yes you are correct.

How do you implement getState though? The process methods receives param changes (through IParameterChanges) and changes its state accordingly after which getState is called by the UI thread. How does process writes this state in a thread safe way without using locks or allocating memory?

Thanks
Yan

Arne Scheffler
Posts: 291
Joined: Mon Jun 20, 2016 7:53 am

Re: Is IComponent::getState()/setState() thread-safe?

Post by Arne Scheffler » Mon Jun 18, 2018 8:26 am

the setState() is indeed much more complex if you want to get this done right. But most of the time you can just make a copy of your data in the beginning of the setState() method without getting interrupted. That said a correct solution involves a lock free ring buffer, where you push changes from the audio thread and read it back from the main/UI thread.

Cheers,
Arne

User avatar
pongasoft
Posts: 84
Joined: Sun Mar 11, 2018 5:57 pm
Location: Las Vegas, USA
Contact:

Re: Is IComponent::getState()/setState() thread-safe?

Post by pongasoft » Mon Jun 18, 2018 9:28 pm

Arne Scheffler wrote:Hi,

Code: Select all

struct MyParams {
	double v1;
	bool v2;
};

class Processor : public AudioEffect
{
//....
	std::atomic<MyParams*> setStateParams;
	std::atomic<MyParams*> toDeleteParams;
	MyParams processParams;
};

tresult process (ProcessData& data)
{
	auto stateParams = setStateParams.exchange (nullptr);
	if (stateParams)
	{
		processParams = *stateParams;
		auto test = toDeleteParams.exchange (stateParams);
		assert (test == nullptr);
	}
//....
}

void cleanupMemory()
{
	auto toDelete = toDeleteParams.exchange (nullptr); // cleanup memory
	if (toDelete)
		delete toDelete;
	toDelete = setStateParams.exchange (nullptr); // cleanup memory
	if (toDelete)
		delete toDelete;
}

tresult setState (IBStream* stream)
{
//....
	cleanupMemory ();
	// allocate new state
	auto stateParams = new MyParams;
	// read state from stream into stateParams !
	setStateParams.exchange (stateParams);
}
tresult setActive (bool state)
{
	if(!state)
		cleanupMemory ();
}
Coded out of my head, so typos may very well happened.
I hope this makes it clear.

Cheers,
Arne.
Hello Arne

I created a small test case with your exact code (wrapped in a mock processor class to run in a test case) using googletest and which as you pointed out is only doing memory management on the ui thread. Here is the test case:

Code: Select all

struct MyParams {
  double v1;
  bool v2;
};

class Processor
{
public:

  std::atomic<MyParams *> setStateParams{nullptr};
  std::atomic<MyParams *> toDeleteParams{nullptr};
  MyParams processParams{0, true};

  ~Processor()
  {
    cleanupMemory();
  }

  void process()
  {
    auto stateParams = setStateParams.exchange(nullptr);
    if(stateParams)
    {
      processParams = *stateParams;
      auto test = toDeleteParams.exchange(stateParams);
      DCHECK_F(test == nullptr);
    }
  }

  void cleanupMemory()
  {
    auto toDelete = toDeleteParams.exchange(nullptr); // cleanup memory
    if(toDelete)
      delete toDelete;
    toDelete = setStateParams.exchange(nullptr); // cleanup memory
    if(toDelete)
      delete toDelete;
  }

  void setState(MyParams const &params)
  {
    cleanupMemory();
    // allocate new state
    auto stateParams = new MyParams{params.v1, params.v2};
    // read state from stream into stateParams !
    setStateParams.exchange(stateParams);
  }
};

TEST(Processor, MultiThreadSafe)
{
  constexpr int N = 10000;

  Processor processor;

  auto ui = [&] {
    for(int i = 0; i < N; i++)
    {
      processor.setState(MyParams{static_cast<double>(i), true});
    }
  };

  auto processing = [&] {
    for(int i = 0; i < N; i++)
    {
      processor.process();
    }
  };

  std::thread uiThread(ui);
  std::thread processingThread(processing);

  uiThread.join();
  processingThread.join();
}

After running a few iterations, the test fails

Code: Select all

Running main() from gtest_main.cc
Stack trace:
5       0x7fff59172c5d thread_start + 13
4       0x7fff5917356d _pthread_body + 0
3       0x7fff591736c1 _pthread_body + 340
2          0x10cdf48e5 void* std::__thread_proxy<std::tuple<std::unique_ptr<std::__thread_struct, std::default_delete<std::__thread_struct> >, pongasoft::VST::Test::Processor_MultiThreadSafe_Test::TestBody()::$_5> >(void*) + 5171          0x10cdf4b64 pongasoft::VST::Test::Processor_MultiThreadSafe_Test::TestBody()::$_5::operator()() const + 520          0x10cdf4e25 pongasoft::VST::Test::Processor::process() + 677

2018-06-18 14:17:04.040 (   0.001s) [            A41B]    test-concurrent.cpp:318   FATL| CHECK FAILED:  test == nullptr   
Process finished with exit code 134 (interrupted by signal 6: SIGABRT)
This demonstrates that the code is actually not thread safe even if there is only 1 ui thread calling setState and 1 processing thread calling process...

I know you said "typos may very well happened" but since I have been struggling so much with this, I have been trying to create my own version without any success whatsoever. Do you know have a version of this code that is thread safe that you could point me too?

This seems to be an incredibly complicated problem and I really think that the VST SDK should provide at a minimum a solution for implementing both getState and setState (which have different requirements) in a thread safe, lock free, memory-allocation-on-the-ui-thread-only fashion...

Thanks
Yan

User avatar
pongasoft
Posts: 84
Joined: Sun Mar 11, 2018 5:57 pm
Location: Las Vegas, USA
Contact:

Re: Is IComponent::getState()/setState() thread-safe?

Post by pongasoft » Tue Aug 28, 2018 3:07 pm

I wanted to close the loop on this topic. Part of the Jamba framework (https://github.com/pongasoft/jamba) and after spending considerable amounts of time on this issue I believe I finally came up with a solution that is indeed thread safe (as long as there is only 1 consumer and 1 producer) lock free and memory allocation free and also has the advantage of avoiding copies entirely since pointers are exchanged... I know it kinda sounds too good to be true so I certainly hope I did not mess it up :)

The code is here https://github.com/pongasoft/jamba/blob ... ncurrent.h in the LockFree namespace. The 2 main classes are

LockFree::SingleElementQueue and LockFree::AtomicValue

Code: Select all

// example

struct Stats {...};

SingleElementQueue<Stats> queue{};

// in 1 thread (ex: RT)
// with copy
Stats stats = computeStats();
queue.push(stats);

// or without copy (use internal pointer)
queue.updateAndPush([this](Stats *oStats) { computeStats(oStats); });

// in another thread (ex: GUI)
// with copy
Stats stats;
if(queue.pop(stats)) { ... };

// or without copy (use internal pointer)
Stats *stats = queue.pop();
if(stats) { ... };

The reason why you can use the internal pointers is because the push and pop side have their own pointers so since each side is called by only 1 thread it is ok to use it...

Although part of Jamba, this file in particular depends on <memory> and <atomic> only so feel free to copy/reuse in your own projects even if you don't want to use Jamba...

Yan

Post Reply