How to lock ValueTrees

I am trying to update my application so that it is thread safe. ValueTree is at the heart of my data model, and I’m planning to give two threads access to it (NOT the audio thread). I’ve read everything I can about the topic, and I’ve settled on the idea that I should lock all functions that deal with ValueTrees around a global mutex. But I’m running into difficulties working out exactly which functions need locking and which don’t.

Consider this sample code.

class Example
{
public:

    void outer(int i) {
        vt.setProperty(intID, i, nullptr);
        middle("abc");
    }

    void middle(String s) {
        innerFunction(s);
    }

private:
    void inner(String s) {
        vt.setProperty(stringID, s, nullptr);
    }

    ValueTree vt; // another thread will have access to this ValueTree
};

My questions is: which of these functions do I need to lock?

  1. all three functions (outer, middle and inner)?
  2. Only the functions which interact directly with the ValueTree (outer and inner)?
  3. Only the public functions (outer and middle)?

Now I’m pretty sure that in this example code, all three would be safe and it wouldn’t make much difference which one I choose. But things are a lot more complicated in my application, where I have hundreds of functions which interact with ValueTrees in one way or another. The safest thing would be to lock all functions, as in option 1. However, this would certainly lead to some redundancy. To reduce this redundancy, I first thought about locking only those functions which have direct interaction with ValueTrees, as in option 2. But there can still be redundancy here, as both an inner and an outer function can end up locked. So next I though that maybe I can get away with locking only the public API functions. Private functions would be guaranteed to be safe, even though they don’t contain a lock, because they can only be called from a locking public function.

Which of these options is the safest? Are there any other rules of thumb that I can use to decide which functions to lock?

Just in case -nothing ensures the example would be thread-safe. outer may be called from one thread while middle is running on another one. That said, I’d have a CriticalSection as a class member and use ScopedLocks on it in every function that uses the tree directly. CriticalSection is a reentrant mutex, it’s ok to go twice into it in the same thread -std::mutex wouldn’t work for that.

Do you mean that none of the above options ensure that the class would be thread safe? Even the one where all three functions are locked?

I am using ReadWriteLock instead of CriticalSection. I am accessing it as a singleton like this:

const ReadWriteLock& getLock()
{
    static const auto lock = ReadWriteLock();
    return lock;
}

and then using scoped locks wherever I need:

ScopedWriteLock l (getLock());

Is this a bad idea?

(Edit: I’m planning on locking functions from both threads like this).

No, sorry, I misread that -I meant nothing ensures it would be thread-safe as it is, without locks. I guess you’ve checked ReadWriteLock already -it allows multiple concurrent readers, which has a cost. Regarding the singleton -are you sure you need the same lock for all instances of the class?

I have one central ValueTree which is sitting in my fundamental data model class. But instances of the same ValueTree are passed around various different classes, so I need to lock across all of them.

I was originally planning to have a separate ReadWriteLock for each individual node, so that the system would only lock around that particular node. Then I realized that this doesn’t work for a tree structure, because you could be editing a node which is simultaneously deleted by its parent, without the node’s lock being able to do anything about it. Designing a lock that take’s into account the state of all parent nodes seems hopelessly complicated, so I thought it would just be best to lock the whole thing whenever it is being read / written to. The lock doesn’t have to be singleton, this is just the simplest solution I could come up with.

I guess you mean you have many ValueTrees across different classes referring to the same underlying objects. Then it may make sense to have a single lock for all of them. It still doesn’t need to be a singleton. Making it a singleton will make actual different instances (with different underlying objects) use the same lock. You may know you have only one instance, but it’s better to avoid singletons unless they’re really needed. This would fail if your app became a plugin, for example.

This is separate from the ReadWriteLock thing -I meant you should use it only if you know different threads need to read the tree at the same time. Otherwise a CriticalSection is much lighter.

Yes, that’s what I meant.

I take your point. I have other options for this, it just involves more pointers. Anyhow, it is fairly easy to switch from one option to the other, so this part isn’t my primary concern right now.

I didn’t realize that ReadWriteLock is relatively heavy–thanks for pointing this out. I suspect it will still be the right choice in my case, since the secondary thread is going to be doing a lot of reading and only a little writing. But I’ll definitely test out both options.

I would still be interested to know your opinion on which functions I should be locking. Should I lock every function that has anything to do with a ValueTree (safe but heavy handed), or is there some rule that I can use to determine the minimum functions which need to be locked?

Well, the best lock is no lock, so if there are parts of the tree that are always written and read by the same thread, you don’t need to synchronize operations on them. For the parts that need synchronization, I’d lock right where ValueTree functions are used. I think it’s the saner, more maintainable way. I wouldn’t worry about the cost of reentry.

Isn’t it an all-or-nothing kind of a thing though? Both of the threads will have the ability to call ValueTree::removeChild(), which means that every function that deals with ValueTrees needs to be safeguarded.

That’s good to hear!

They theoretically could or they will? If they will, then yes, the whole tree is affected. If you really have a dynamically, concurrently changing structure, that’s quite a beast. There are many possible solutions to this kind of problem, both with and without locks, but it’s all very case-dependent.

A beast indeed!

My ValueTree structure contains lots of user input Strings which the system parses as scripting commands. This is the process I wanted to separate from the message thread–the parsing is fairly CPU intensive, and runs very frequently on a 50ms timer. Some of these commands will alter the structure of the tree, so it definitely will call removeChild().

I thought about using a lock free solution as in this post:

However, I’m not sure that it would actually be any easier to implement, and the asynchronicity might introduce other complications.

Of course, the other solution is to keep the whole thing running on the message thread (if it aint broke, don’t fix it). But the parsing is quite heavy, and it seems like multi-threading could really improve performance here.

If I understand the problem correctly, I’d consider not reading/writing from the ValueTree from multiple threads.

Instead, have the side thread process a regular (non-VT) variable, and when that thread finishes, signal the message thread to write the result back to the VT.

That’s an interesting idea. I can see how I would get data out of the second thread and back to the message thread ValueTree. It’s a bit harder to see how to get data on to it. The parsing function can draw from any node of the ValueTree, so it’s a bit hard to define straightforward input.

Maybe I could construct a parallel ValueTree on the second thread, which mirrors the original and updates asynchronously from the message thread. Then the second thread can use that one for input and send its data back to the message thread when it finishes, as you suggested.

For non-audio thread contexts, this method I use is:

   MessageManager::callAsync ([this, data]()
   {
       myValueTree.setproperty(TheDataPropertyId, data, nullptr);
   });

It’s really much simpler than that - every time you launch your side thread you launch it with a copy of the ValueTree.

Then as @cpr2323 mentioned you can call an async function that will copy the modified tree back into the original tree on the message thread.

P.S. I do think that if you’re looking for speed, you should copy your data to a format that isn’t ValueTree before doing the expansive operation.

just for clarity, launch it with a reference of the ValueTree :slight_smile: I am just pointing out that copying a ValueTree object is actually taking a reference to the underlying ValueTree. If one wants an actual copy (which is disconnected from the original ValueTree) you use ValueTree::createCopy

Sorry, but I don’t really follow. If the second thread has a reference of a copy of a ValueTree which belongs on the message thread, then the second thread might read the underlying data at the same time as the message thread writes it. Isn’t that what we’re trying to avoid?

No - on the message thread you make the copy, launch the side thread with the copy and move on. When the second thread finishes, it passes the copy to the message thread. Then later in the message thread, the copy gets merged into the original.

You mean a deep copy using createCopy(), right? I can see that this would work, but it would also make for a lot of copying.

Yes, you pay for the extra copies but remove the need for complicated thread syncing which in almost all cases would be better.

Of course, you can only copy the tree chunk needed for the operation.