How to lock ValueTrees

General JUCE discussion
21

I actually meant a deep copy (using createCopy, as you mentioned). Since a reference/copying the reference counted pointer would cause many thread contentions when reading/writing from the VT, which I think the OP is trying to avoid (for a good reason).

22

@eyalamir, yeah I saw your further explanation… cool solution. :slight_smile:

23

I don’t think you can escape making deep copies -otherwise you’d have your data changed from the other side while it’s being used. Also, consider not having everything in the same place. What is written by the message thread and read by the parser should be separate from what is written by the parser and read by the message thread -otherwise your state will get corrupted.

24

Please don‘t. The ValueTree is a wrapper around a ReferenceCountedObject. Copying a ValueTree (not deep copy) is a lightweight operation. And holding a reference to a referenceCountedObject defeats the purpose and risks dangling references, especially since we are talking of multi threading!

The only situation where you need a reference to a ValueTree is when you need to add or remove Listeners, which is added to that Wrapper object, not the SharedObject.

1 Like
25

You missed my clarification… :slight_smile: I did a poor job of saying exactly what you just said.

26

Then I didn‘t understand the clarification, sorry :wink:

A reference to a ValueTree as I understand it is ValueTree& which is no good…

1 Like
27

Thanks for the suggestions folks. The idea of taking a deep copy for the second thread is a good one. It’s more complicated in my situation though, since the second thread will need to have access to the whole ValueTree hierarchy at any given time. It might be possible for me to do it by having a shadow version of the whole ValueTree updated asynchronously on the second thread. However, before I proceed with this, I want to think through the locking option one last time.

The essence of my original question was, “should I lock things at a high level or at a low level?” The conventional advice seems to be, lock at a low level (ie., lock any function which has a direct interaction with a ValueTree). However, this approach still seems prone to inconsistencies, as a low level get() function could return a value which quickly becomes out of date, even though the function is itself locked.

So instead, I’m thinking that it would be a good idea to lock things at a high level, and create a “perimeter of defense” around the inner workings of the program.

Suppose that I were to identify all of the high-level actions that can lead to can lead to ValueTree interaction and then lock those all around the same mutex. These would be functions that sit at the very top of the API–things like enterUserText() and getCurrentState()–which can lead to very complicated lower level function calls. But because they’re all locked at a high level, it means that no two threads can enter them at the same time.

As I see it, the advantages of would be:

  • You can ensure thread safety with a minimum number of locks.
  • If you’re careful, you can probably manage all of the locking functions together in a single class.
  • You don’t need to worry about what happens at a lower level, so long as you have the higher levels are in order.

The disadvantage would be that the system spends more time locked, but this price might be worth the simplicity that comes with it.

Am I right in thinking that this would be a good approach to thread safety?

28

If you need the state to remain consistent through a whole operation, you’ll need a mutex for the whole operation indeed. That’s why it’s easier to take a copy -you work on a snapshot and merge the result at the end. We don’t know what your data structure is, but I sense this is being complicated by having everything together. As I said before, the copy / merge back thing won’t work if you don’t separate the input and the output of the parser. Suppose you have some data, let’s call it x, copied from the tree. The parser gives back another tree, also containing x, which you’ll merge later on the message thread. In the meanwhile, x has been modified in the original tree, from user input. The merge will overwrite this value with the older one, unless you keep track of the change somehow. This is a needless complication, because it seems from your description that the input and the output of the parser are actually disjoint sets, which happen to be stored in the same place by choice.

Using a mutex seems much less maintainable, especially with a big single tree. You’ll have to lock every access. It probably won’t be more performant either. You can reduce the data to and from the parser to what it actually needs, and make it all wait-free.

1 Like
29

Your reply makes me more inclined to use locking!

The parser has the ability to modify the ValueTree– that’s fundamental to my design. So node A might contain the text “set node B to node C”, and the parser would carry out this instruction, which involves reading text from node A, fetching a value from node C, and writing to node B. This is pretty complicated stuff, and the more I think about it, and I’m not sure that the copying solution is going to work, especially given the merging problem you brought up.

Just to be clear, is the high-level locking solution bad because the functions will spend so much time locked? Or is it bad for other reasons?

30

I see. So the user and the parser can modify the same data at the same time. Either you lock, or use a queue.

It’s certainly problematic. If the parser is launched right when the user makes a change, the message thread will lock for the whole parsing, which kills the point of having a separate thread. You’d have to test it with heavy interaction and see what happens. If you use a queue, it should be MPSC and have an entry for each node change.

31

I really think you’re overcomplicating it.
There’s no problem at all with copying a massive data structure, making tons of internal changes on the side thread, and then copying it back to the message thread which would ‘merge’ it by copying the thing back in.

Your data handling objects (ValueTree Listeners, etc) should be ‘ready’ for these deep changes, no matter how they arrive (side thread/preset loading, etc). Once you eliminate the threading contention, the message thread ‘merge’ would be identical to loading the entire tree from XML, etc, and then notify all listeners.

Any other solution would create a ‘tearing’ of the data which you will not be able to protect, unless you choose to use a data structure which isn’t ValueTree (and even in that case locking might be very complicated, as you need to make sure you never give access to data that could be teared by the side thread even with locks).

32

As for the problem of sending something to the side thread while the user can make more interactions: You could use a simple lock to signal that you’re in the middle of the side thread action and block some high level actions.

That however would be a much simpler process since you’re only blocking high level operations (say, actions called from mouseDown) and not internal tree access.

33

I’m thinking that it would be better to disable GUI input for the nodes while the parser is running. That way you wouldn’t block the message thread and wouldn’t even need a mutex. Just raise a “parsing” flag to ignore GUI events for the nodes. (Thinking a bit more, there’s still the case of the parser launching while the user is editing. Well… just an idea.)

1 Like
34

Disabling input is a good idea.

Although it could be frustrating for the user to ‘lose’ mouse clicks, so I’d prefer to delay them for a little bit (while waiting for the lock), unless this is a really long operation that could involve file loading, etc, where it might even be clearer to just block everything.

I do agree that sometimes using a simple atomic flag to indicate if a high level operation is allowed is really the best way to go, as you can even a single top level component manage that and abstract this problem from your chain of sub components.

35

It would be ok if it were mouse clicks, but these are strings with commands -disabling textboxes while they’re being edited seems a bit convoluted…

I think I’d do this. You have an atomic flag, a main tree, and a UI tree. All user input for the nodes goes to the UI tree, the parser works on the main tree. The flag starts up. In a (msg thread) Timer callback, if the flag is up you merge the UI tree into the main one, clear the UI tree, and launch the parser. When it finishes, it raises the flag.

(Or, in std::atomic_flag terms: the flag starts down, if !flag.test_and_set etc, when the parser finishes it clears the flag.)

1 Like
36

@kamedin I’m not sure that I understand your suggestion. If the flag is used to disable user input while the parser IA working, then why bother with the copying?

The thing is that the parser is running on a 50ms Timer. Copying all that data 20 times a second seems like it might add up.

37

Another option is to have a shadow version of the ValueTree on the parsing thread which stays in sync with the main one on the message thread. The parsing thread thread would look something like this:

while (!threadShouldExit())
{
    updateChangesToValueTreeFromMessageThread(); // step 1
    parse();                                     // step 2
    mergeValueTreeBackIntoMessageThread();       // step 3
}

I already have step 1 working as prototype–I’ve used ValueTreeSynchroniser and used AbstractFifo to create an asynchronous, lock-free updater that updates the ValueTree without having to copy the whole thing. Step 3 I’m not sure about yet, and I’d need to work out something for the merging problem them Kamedin brought up.

38

I’m not sure that I understand your suggestion. If the flag is used to disable user input while the parser IA working, then why bother with the copying?

The message thread is used for tons of things usually, not just for mouse input. You might be updating meters on the message thread, transports and other things that you don’t want to ‘freeze’.

The thing is that the parser is running on a 50ms Timer. Copying all that data 20 times a second seems like it might add up.

First of all I think you’re underestimating your computer’s ability to copy stuff. Unless this is gb of data, the computer can copy it quite easily.

In reality though, I’m assuming you’d only do the parsing if something changed, and only once?

I already have step 1 working as prototype–I’ve used ValueTreeSynchroniser and used AbstractFifo to create an asynchronous, lock-free updater that updates the ValueTree without having to copy the whole thing.

All a synchronizer does is sync one tree to another. It won’t solve your threading problems though - during that sync (on the the message thread) your side thread would run and the (synchronized) ValueTree might not finish updating, so you’d get corrupt information. I’m assuming running Xcode Thread Sanitizer would catch this as a problem right away.

39

That’s right, although it might need to access the ValueTree to know whether it needs to parse or not. I’m not sure about this though–I’ll have to take a closer look.

I was using a lock-free Fifo to transfer the synchronization data from one thread to other. It has passed my initial tests, although I haven’t tested it rigorously. Code is here if anyone wants to take a look.

40

No, I was suggesting something else. No locking, but also no copying. You have two ValueTrees, and use one of them to temporarily store changes from user input until the next timer callback. The flag indicates if the parser is running. If it’s not, you merge the UI tree into the main one, clear the UI tree, and launch the parser.

every x time on the message thread
    if parser is not running
        copy children and properties from UItree to mainTree
        clear UItree
        launch parser

copyPropertiesAndChildrenFrom() won’t work, because you need to keep all children and properties in mainTree that are absent in UItree. The point with this is that mainTree writes from the message thread are guaranteed to happen when the parser is not running, so the parser always sees a coherent state.

Even better:

every x time on the message thread
    if parser is not running and UItree is not empty
        copy children and properties from UItree to mainTree
        clear UItree
        launch parser